Implement support for watching directories containing implicitly analyzed files

Implement support for watching directories containing implicitly analyzed files

R=paulberry@google.com

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

[Brian Wilkerson, 2015-07-18 21:50:16]

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

[Paul Berry, 2015-07-19 18:38:34]

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

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:61: * that folder with the given
[token].
Documentation should say what happens if the same folder is watched multiple
times with the same token.  Are the later calls to addFolder() ignored, is the
folder effectively watched N times, or is the caller required to ensure that
this situation does not arise?  (I think that the later calls are effectively
ignored, since WatchNode.tokens is a set).

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:84: folderNode.subscription =
folder.changes.listen((WatchEvent event) {
How about just:

folderNode.subscription = folder.changes.listen(_handleWatchEvent);

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:93: if (childNode.subscription !=
null) {
Consider adding:

assert(childNode.subscription != null);

Since the only way a childNode would have a null subscription would be due to a
bug in the WatchManager.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:104: */
I like the fact that nothing happens if removeFolder() is called on a
folder/token pair that are not currently being watched.  That's good defensive
programming, however it carries the risk of masking bugs in clients.

Suggestion: document that the caller is required to call removeFolder() on a
folder/token pair that are currently being watched, and add assertions to verify
that folderNode != null and tokens.remove(token) returns true.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:119: // subscriptions for all of
it's children and cancel it's subscription.
Nit: both "it's" should be "its".

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:123: childNode.subscription =
childNode.folder.changes
Consider adding "assert(childNode.subscription == null);" prior to this line.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:124: .listen((WatchEvent event) {
Again, ".listen(_handleWatchEvent);" should be sufficient.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:143: //    void
visitNode(WatchNode node) {
Was this commented-out code left here by accident?  It should either be removed
or its purpose should be documented with a TODO comment.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:151: WatchNode<T> parent =
rootNode.findParent(path);
What happens if path is identical to the path to one of the folder being
watched?  (E.g. we are watching "/foo/bar", and "/foo/bar" gets deleted from the
filesystem).  I believe your algorithm will only inform watchers for parent
folders.  But I think clients will expect to also receive notifications for the
folder itself, since AFAIK this is the behavior of the existing "watcher"
package.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:174: static List<WatchNode>
EMPTY_LIST = <WatchNode>[];
I think this goes away if you take my suggestion below.  However, if you don't,
please make it const.  As it stands there's too much danger of someone
accidentally adding an entry to this list and breaking a bunch of invariants.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:181: Folder folder;
Can this be final?

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:191: List<WatchNode> _children =
null;
It seems like premature optimization to use `null` for leaf nodes.  Consider
making this variable final and initializing it to <WatchNode>[].  That should
eliminate a bunch of explicit null checks (and might even allow the VM to drop
implicit null checks, since the VM will be able to prove that this field is
never null).

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:196: Set<T> tokens = new
HashSet<T>();
This can be final.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:213: List<WatchNode> get children
=> _children == null ? EMPTY_LIST : _children;
Consider changing the return type to Iterable<WatchNode> so that clients are
discouraged from modifying the list directly and disrupting invariants.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:221: }
Consider setting parent to null so the node returns to its initial state.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:250: void forEachChild(void
function(WatchNode<T> node)) {
This method is only called from commented-out code.  Can it be removed?

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:304: }
Consider setting child._children to null so that there is less danger of
corrupting data structures if the caller tries to re-use the child.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/test/sr...
File pkg/analysis_server/test/src/watch_manager_test.dart (right):

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/test/sr...
pkg/analysis_server/test/src/watch_manager_test.dart:190: await new
Future.delayed(Duration.ZERO);
Consider using pumpEventQueue(), which makes the test less brittle by letting
the event queue run multiple times.

[Brian Wilkerson, 2015-07-19 21:24:21]

PTAL

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

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:61: * that folder with the given
[token].
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:84: folderNode.subscription =
folder.changes.listen((WatchEvent event) {
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:93: if (childNode.subscription !=
null) {
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:104: */
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:119: // subscriptions for all of
it's children and cancel it's subscription.
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:123: childNode.subscription =
childNode.folder.changes
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:124: .listen((WatchEvent event) {
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:143: //    void
visitNode(WatchNode node) {
It was missed while cleaning-up.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:151: WatchNode<T> parent =
rootNode.findParent(path);
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:174: static List<WatchNode>
EMPTY_LIST = <WatchNode>[];
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:181: Folder folder;
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:191: List<WatchNode> _children =
null;
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:196: Set<T> tokens = new
HashSet<T>();
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:213: List<WatchNode> get children
=> _children == null ? EMPTY_LIST : _children;
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:221: }
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:250: void forEachChild(void
function(WatchNode<T> node)) {
Done

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/watch_manager.dart:304: }
I can't because now there's an assumption that _children is never null. The only
caller is WatchManager, and it doesn't re-use WatchFolders, but the code now
removed the grandchild from the child's list of children just to avoid potential
problems.

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/test/sr...
File pkg/analysis_server/test/src/watch_manager_test.dart (right):

https://codereview.chromium.org/1244613004/diff/1/pkg/analysis_server/test/sr...
pkg/analysis_server/test/src/watch_manager_test.dart:190: await new
Future.delayed(Duration.ZERO);
Done

[Paul Berry, 2015-07-19 22:49:08]

lgtm assuming the issue about modifying child.children during iteration is
addressed.

Your call on the other comments--I suspect we could have a lively in-person
discussion abut them, but I don't want to hold up this CL waiting for us to have
a chance to do that :)

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/watch_manager.dart (right):

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/watch_manager.dart:93: assert(childNode.subscription
!= null);
Perhaps I should have been clearer about my suggestion because I'm not convinced
this is an improvement.  I liked the defensiveness of the "if" statement you had
in the previous version, because it meant that if a bug ever caused
childNode.subscription to be null, we would have avoided an exception in
production mode.  But of course the downside of defensive programming is that it
masks bugs, and it seemed to me that we could have our cake and eat it too by
adding an assertion (and making no other change) so that in production mode we
remain defensive, but in checked mode, the bug is detected as early as possible.

As the code stands now, a null childNode.subscription will cause an exception in
both production and checked modes.  So we've lost our defensiveness, and in
point of fact the assertion is now redundant (since if childNode.subscription is
null, the very next line is guaranteed to throw an exception).

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/watch_manager.dart:110: throw new StateError('Folder
(${folder.path}) is not being watched.');
I have a similar concern here.  By throwing an exception (rather than adding an
assertion) we gain the ability to detect bugs earlier but we lose the
defensiveness.  Personally, I would prefer to keep the old defensive behavior in
production mode, and simply add assertions so that in checked mode, we detect
bugs sooner.

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/watch_manager.dart:281:
child._children.remove(grandchild);
I don't think this is safe, since it causes child.children to be modified while
iteration is still in progress.  In fact, I'm kind of surprised this didn't
cause an exception during unit testing (although the docs for List.iterator
simply say that modifying the list during iteration *may* cause an error, so
maybe we just got "lucky".)

[Brian Wilkerson, 2015-07-20 15:52:17]

PTAL

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/watch_manager.dart (right):

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/watch_manager.dart:93: assert(childNode.subscription
!= null);
Ok. The 'if' is restored.

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/watch_manager.dart:110: throw new StateError('Folder
(${folder.path}) is not being watched.');
Ok. I've put the old code back and added the assertions, but they look kind of
strange because they're both of the form "assert(false)". Also, I dislike the
lack of a message because it will be harder to debug.

https://codereview.chromium.org/1244613004/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/watch_manager.dart:281:
child._children.remove(grandchild);
No, I forgot to re-run the tests. I did fail. They're passing now.

[Paul Berry, 2015-07-20 15:54:39]

lgtm

[Brian Wilkerson, 2015-07-20 16:04:42]

Committed patchset #3 (id:40001) manually as
bc265ebe28e26d290e9e5c28e905dc8c747e1a8f (presubmit successful).