watch for file system changes after context created

watch for file system changes after context created

Closes #23671

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/13b601ffc526fbdc58e73467ff745d1d3c18e47f

[danrubel, 2015-06-18 14:19:09]

danrubel@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2015-06-18 20:24:42]

https://codereview.chromium.org/1188663006/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1188663006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:452: // (e.g.
_computePackageUriResolver throws an exception)
I don't understand why this is necessary.  Subscribing to file changes before
the context has been set is safe; what's unsafe is *processing* the file changes
after the context has been set.  But that's ok because we don't process change
notifications until the next time the event loop runs, so even without this CL,
we should be fine--by the time we process the notification, the context will
have been set.  Are you quite sure that this is what's going on with bug 23671? 
It seems like it can't possibly be what's happening.

Delaying the subscription until after creating the context has an additional
disadvantage: it creates a race condition.  If the filesystem changes after
creating the context but before we start listening for changes, we'll miss those
changes.  This isn't just an academic concern--we see exactly these kinds of
issues when someone is doing a git operation like a rebase and it causes the
analyzer to rebuild the context.

[danrubel, 2015-06-22 11:22:56]

Reworked per discussion. PTAL.

https://codereview.chromium.org/1188663006/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1188663006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/context_manager.dart:452: // (e.g.
_computePackageUriResolver throws an exception)
On 2015/06/18 20:24:42, Paul Berry wrote:
> I don't understand why this is necessary.  Subscribing to file changes before
> the context has been set is safe; what's unsafe is *processing* the file
changes
> after the context has been set.  But that's ok because we don't process change
> notifications until the next time the event loop runs, so even without this
CL,
> we should be fine--by the time we process the notification, the context will
> have been set.  Are you quite sure that this is what's going on with bug
23671? 
> It seems like it can't possibly be what's happening.
> 
> Delaying the subscription until after creating the context has an additional
> disadvantage: it creates a race condition.  If the filesystem changes after
> creating the context but before we start listening for changes, we'll miss
those
> changes.  This isn't just an academic concern--we see exactly these kinds of
> issues when someone is doing a git operation like a rebase and it causes the
> analyzer to rebuild the context.

Good point. Per discussion I reworked this to add the watcher first, then
unsubscribe if there is an exception before the context is assigned.

[Paul Berry, 2015-06-26 15:30:21]

Whoops!  Sorry for forgetting about this.  LGTM.

[danrubel, 2015-06-29 16:24:32]

Committed patchset #4 (id:60001) manually as
13b601ffc526fbdc58e73467ff745d1d3c18e47f (presubmit successful).