Issue 30238. Fix for subscribing for a notifications with already cached analysis result.

Issue 30238. Fix for subscribing for a notifications with already cached analysis result.

With the new 'sendCachedToStream' option we can control that whether
cached analysis result is also reported into the 'results' stream when
it is desirable (when we do this to send analysis notifications from
the stream listener). Or not, when it is not desirable, e.g. when we
ask becaue we need the results unit for Quick Assists.

R=brianwilkerson@google.com
BUG= https://github.com/dart-lang/sdk/issues/30238

Committed: https://github.com/dart-lang/sdk/commit/939d8aaf6dd4a21cb5d0721cec3b90562b3c5b14

[Brian Wilkerson, 2017-07-25 16:43:51]

lgtm, with two minor reservations.

First, I think this means that if a client subscribes for one notification for a
given file, and then subscribes for two notifications for that same file, that
we will re-send the data for the first subscription, even though nothing has
changed. That probably doesn't happen often enough to worry about, though.

Second, I don't particularly like this as a public API (feels like a leaky
abstraction), so I'm hoping we don't need to make this part of the
AnalysisSession API.

[scheglov, 2017-07-25 17:19:19]

On 2017/07/25 16:43:51, Brian Wilkerson wrote:
> lgtm, with two minor reservations.
> 
> First, I think this means that if a client subscribes for one notification for
a
> given file, and then subscribes for two notifications for that same file, that
> we will re-send the data for the first subscription, even though nothing has
> changed. That probably doesn't happen often enough to worry about, though.

Yes, and actually I think this has been since the switch to Analysis Driver.


> Second, I don't particularly like this as a public API (feels like a leaky
> abstraction), so I'm hoping we don't need to make this part of the
> AnalysisSession API.

Yes, this is not very nice.

Unfortunately we cannot always report cached results into the stream, because in
this case we would resend notifications when the user navigates through the same
file.

We also cannot use getResult().then(sendNotifications) in
setAnalysisSubscriptions, because if the result is NOT cached, we would process
it and send notifications twice - because we received it in the stream, and
because we process the return value of getResult(). We could try to expose and
compare the result signature in Analysis Server, but that's not nice too.

So, at the end it seemed better for me to add a flag to report the result to the
stream anyway, whether it is cached or freshly computed, but only once.

[Brian Wilkerson, 2017-07-25 17:22:30]

Yeah, I don't have a great solution in mind either. Better to get the fix in and
then think about the issues when (and if) they become more important.

[scheglov, 2017-07-25 17:28:11]

Description was changed from

==========
Issue 30238. Fix for subscribing for a notifications with already cached
analysis result.

With the new 'sendCachedToStream' option we can control that whether
cached analysis result is also reported into the 'results' stream when
it is desirable (when we do this to send analysis notifications from
the stream listener). Or not, when it is not desirable, e.g. when we
ask becaue we need the results unit for Quick Assists.

R=brianwilkerson@google.com
BUG= https://github.com/dart-lang/sdk/issues/30238
==========

to

==========
Issue 30238. Fix for subscribing for a notifications with already cached
analysis result.

With the new 'sendCachedToStream' option we can control that whether
cached analysis result is also reported into the 'results' stream when
it is desirable (when we do this to send analysis notifications from
the stream listener). Or not, when it is not desirable, e.g. when we
ask becaue we need the results unit for Quick Assists.

R=brianwilkerson@google.com
BUG= https://github.com/dart-lang/sdk/issues/30238

Committed:
https://github.com/dart-lang/sdk/commit/939d8aaf6dd4a21cb5d0721cec3b90562b3c5b14
==========

[scheglov, 2017-07-25 17:28:12]

Committed patchset #1 (id:1) manually as
939d8aaf6dd4a21cb5d0721cec3b90562b3c5b14 (presubmit successful).