Wrap Directory.watch on linux for the watcher package.

Wrap Directory.watch on linux for the watcher package.

R=rnystrom@google.com
BUG=14428
Committed: https://code.google.com/p/dart/source/detail?r=30081

[Bob Nystrom, 2013-11-06 19:24:03]

https://codereview.chromium.org/46843003/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/46843003/diff/1/pkg/pkg.status#newcode381
pkg/pkg.status:381: watcher/test/*: StaticWarning
What's this for? Can it be fixed?

https://codereview.chromium.org/46843003/diff/1/pkg/pkg.status#newcode387
pkg/pkg.status:387: watcher/test/*/linux_test: Pass, Slow # Issue 14606
Before only no_subscription_test was flaky. Now all of them are?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:45: /// Creates a new
[DirectoryWather] monitoring [directory].
"Wather" -> "Watcher".

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/linux.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:51: final _subWatchers = new
Map<String, _LinuxDirectoryWatcher>();
"sub" isn't a word (in this context), so it should be "_subwatchers". Maybe
"_subdirectoryWatchers"?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:71: _watchSubDir(entity.path);
"_watchSubdirectory" or at least "_watchSubdir"

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:74: _eventsController.close();
I'm guessing you close here because if the listing fails, you can't correctly
tell the state of the directory contents so can't function correctly. Should
document that.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:109:
files.addAll(watcher._allFiles);
This creates and discards a lot of intermediate sets. It may be more efficient
to split this out into a getter that creates one set for the result and a
recursive helper that takes it in so you just create one set for the entire
traversal.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/resubscribable.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/resubscribable.dart:43: /// Creates a new
[ResubscribableDirectoryWatcher] wrapping the watcher
"watcher" - "watchers".

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/resubscribable.dart:57: // it's called,
since a new completer may be created if this subscription
This comment is confusing me. I'm not sure what the first "this" and the second
"it's" refer to. Can you clarify it?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dart
File pkg/watcher/lib/src/utils.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:38: }
We have an increasingly large set of copy/pasted functions. We should look into
adding them to quiver and just re-using that.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:44: /// A stream transformer that batches all
events sent in adjacent event loops.
I don't know what "adjacent event loops" means. Document this in more detail
please.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:53: Timer.run(() {
Document what's going on here.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/watcher.dart
File pkg/watcher/lib/watcher.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/watcher.dart#...
pkg/watcher/lib/watcher.dart:9: export 'src/directory_watcher/polling.dart';
I don't think we should export this.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
File pkg/watcher/test/directory_watcher/linux_test.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/linux_test.dart:28: test('DirectoryWatcher
creates a LinuxDirectoryWatcher on Linux', () {
I don't think this should be part of the public API. From the user's
perspective, they should only know they got a DirectoryWatcher.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:103: deleteFile("file.txt");
Why doesn't this notify?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:193: renameDir("dir/sub", "sub");
deleteDir()?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:197:
expectRemoveEvent("dir/sub/sub-$i/sub-$j/file-$k.txt"));
Do you test that you *don't* receive spurious events? Here since you're doing a
rename, I would expect a bunch of add events too. Do they not cause the test to
fail?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart
File pkg/watcher/test/utils.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:183: var future = _unorderedEventFuture;
Document that this is being stored locally since _unorderedEventFuture may get
assigned before the schedule() call below runs.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:184: _nextEvent++;
Move this above the if() and remove the ++ below.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:188: completion(contains(matcher)));
For inAnyOrder, should we care about duplicate events? It looks like this
doesn't as far as I can tell.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:257: /// Schedules creating a directory in the
sandbox at [path].
Can you use the descriptor API instead of adding this?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:265: void renameDir(String from, String to) {
Does scheduled_test have something for this already? Should it?

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:273: void deleteDir(String path) {
Ditto.

[nweiz, 2013-11-07 00:46:36]

https://codereview.chromium.org/46843003/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/46843003/diff/1/pkg/pkg.status#newcode381
pkg/pkg.status:381: watcher/test/*: StaticWarning
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> What's this for? Can it be fixed?

This is the same warning that was there before. I've just changed the matcher so
it applies to the new tests. I'm not sure what the actual warning is.

https://codereview.chromium.org/46843003/diff/1/pkg/pkg.status#newcode387
pkg/pkg.status:387: watcher/test/*/linux_test: Pass, Slow # Issue 14606
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Before only no_subscription_test was flaky. Now all of them are?

This isn't a flaky annotation, it's a slow annotation. The tests are slow due to
issue 14606.

I removed the flaky annotation for no_subscription_test because your comment on
issue 13705 seemed to indicate that the flakiness should be taken care of.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:45: /// Creates a new
[DirectoryWather] monitoring [directory].
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> "Wather" -> "Watcher".

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/linux.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:51: final _subWatchers = new
Map<String, _LinuxDirectoryWatcher>();
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> "sub" isn't a word (in this context), so it should be "_subwatchers". Maybe
> "_subdirectoryWatchers"?

It's not a word, but it is a prefix; the English here would be "sub-watchers",
not "subwatchers".

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:71: _watchSubDir(entity.path);
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> "_watchSubdirectory" or at least "_watchSubdir"

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:74: _eventsController.close();
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> I'm guessing you close here because if the listing fails, you can't correctly
> tell the state of the directory contents so can't function correctly. Should
> document that.

I'd say it's more general than that; we close here because it's an unexpected
error and unexpected errors should generally take down the code that's emitting
them.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:109:
files.addAll(watcher._allFiles);
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> This creates and discards a lot of intermediate sets. It may be more efficient
> to split this out into a getter that creates one set for the result and a
> recursive helper that takes it in so you just create one set for the entire
> traversal.

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/resubscribable.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/resubscribable.dart:43: /// Creates a new
[ResubscribableDirectoryWatcher] wrapping the watcher
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> "watcher" - "watchers".

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/resubscribable.dart:57: // it's called,
since a new completer may be created if this subscription
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> This comment is confusing me. I'm not sure what the first "this" and the
second
> "it's" refer to. Can you clarify it?

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dart
File pkg/watcher/lib/src/utils.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:38: }
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> We have an increasingly large set of copy/pasted functions. We should look
into
> adding them to quiver and just re-using that.

I'd love to, but there are a bunch of hurdles we have to jump before pub or a
pkg package can start using a package outside of the repo.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:44: /// A stream transformer that batches all
events sent in adjacent event loops.
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> I don't know what "adjacent event loops" means. Document this in more detail
> please.

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:53: Timer.run(() {
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Document what's going on here.

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/watcher.dart
File pkg/watcher/lib/watcher.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/watcher.dart#...
pkg/watcher/lib/watcher.dart:9: export 'src/directory_watcher/polling.dart';
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> I don't think we should export this.

I think we should allow users to explicitly use the polling watcher if they
want. Native watchers are always going to involve some behavioral compromises
that users may not want to buy into.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
File pkg/watcher/test/directory_watcher/linux_test.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/linux_test.dart:28: test('DirectoryWatcher
creates a LinuxDirectoryWatcher on Linux', () {
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> I don't think this should be part of the public API. From the user's
> perspective, they should only know they got a DirectoryWatcher.

It's not part of the public API -- we only get LinuxDirectoryWatcher in scope
because we're importing watcher/src/directory_watcher/linux. It's important that
we properly detect the OS, though, so I think this test is worthwhile.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:103: deleteFile("file.txt");
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Why doesn't this notify?

Because the file is no longer there, and we know that when we fire the event. We
shouldn't fire an ADD event for a file we know doesn't exist.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:193: renameDir("dir/sub", "sub");
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> deleteDir()?

Deleting a directory recursively is different than moving it out of the
directory. Recursive deletion usually involves deleting the contents and then
the directory in quick succession, whereas moving it just makes the directory
and all its contents go away all at once.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:197:
expectRemoveEvent("dir/sub/sub-$i/sub-$j/file-$k.txt"));
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Do you test that you *don't* receive spurious events?

Yes; my change to [startWatcher] asserts that no unexpected events are
generated.

> Here since you're doing a
> rename, I would expect a bunch of add events too. Do they not cause the test
to
> fail?

The directory is being renamed out of the watched directory, so it looks to the
watcher like it's being deleted.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart
File pkg/watcher/test/utils.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:183: var future = _unorderedEventFuture;
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Document that this is being stored locally since _unorderedEventFuture may get
> assigned before the schedule() call below runs.

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:184: _nextEvent++;
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Move this above the if() and remove the ++ below.

Done.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:188: completion(contains(matcher)));
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> For inAnyOrder, should we care about duplicate events? It looks like this
> doesn't as far as I can tell.

I thought about doing so, but I decided it wasn't worth the trouble. Since we
validate the number of events sent, the only case where we break is if a test
tries to assert that multiple identical events were fired, which doesn't seem
useful.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:257: /// Schedules creating a directory in the
sandbox at [path].
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Can you use the descriptor API instead of adding this?

I wanted to keep this consistent with the other methods in this file, which
don't use the descriptor API. I'll add a TODO to switch these tests over to
using descriptor if you like, but to be honest I don't think it's a great fit.
These tests usually create only a couple files or directories per test, and very
rarely create the sort of complex directory structure that the descriptor API is
designed to make easy. I think there's more value in preserving similarity
between the creation functions and the modification functions in this case than
in using the descriptor library for the creation functions.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:265: void renameDir(String from, String to) {
On 2013/11/06 19:24:04, Bob Nystrom wrote:
> Does scheduled_test have something for this already? Should it?

It doesn't. Right now it just exposes the descriptor API, which exists more to
provide a clear way of setting and validating complex directory structures than
to provide a general-purpose "scheduled IO" library. It's possible that there's
room for a new library that just takes common filesystem IO operations and
schedules them, but it would look more like a collection of utility functions
than the wrappers that exist now do.

[Bob Nystrom, 2013-11-07 18:16:04]

One suggestion but LGTM!

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/linux.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/linux.dart:51: final _subWatchers = new
Map<String, _LinuxDirectoryWatcher>();
On 2013/11/07 00:46:37, nweiz wrote:
> On 2013/11/06 19:24:04, Bob Nystrom wrote:
> > "sub" isn't a word (in this context), so it should be "_subwatchers". Maybe
> > "_subdirectoryWatchers"?
> 
> It's not a word, but it is a prefix; the English here would be "sub-watchers",
> not "subwatchers".

SGTM.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dart
File pkg/watcher/lib/src/utils.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/src/utils.dar...
pkg/watcher/lib/src/utils.dart:38: }
On 2013/11/07 00:46:37, nweiz wrote:
> On 2013/11/06 19:24:04, Bob Nystrom wrote:
> > We have an increasingly large set of copy/pasted functions. We should look
> into
> > adding them to quiver and just re-using that.
> 
> I'd love to, but there are a bunch of hurdles we have to jump before pub or a
> pkg package can start using a package outside of the repo.

Some of those hurdles have been jumped already (for example, we bring in the
markdown package now), but we don't have to worry about this right now. Just
something to consider.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/watcher.dart
File pkg/watcher/lib/watcher.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/lib/watcher.dart#...
pkg/watcher/lib/watcher.dart:9: export 'src/directory_watcher/polling.dart';
On 2013/11/07 00:46:37, nweiz wrote:
> On 2013/11/06 19:24:04, Bob Nystrom wrote:
> > I don't think we should export this.
> 
> I think we should allow users to explicitly use the polling watcher if they
> want. Native watchers are always going to involve some behavioral compromises
> that users may not want to buy into.

That makes sense.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:193: renameDir("dir/sub", "sub");
On 2013/11/07 00:46:37, nweiz wrote:
> On 2013/11/06 19:24:04, Bob Nystrom wrote:
> > deleteDir()?
> 
> Deleting a directory recursively is different than moving it out of the
> directory. Recursive deletion usually involves deleting the contents and then
> the directory in quick succession, whereas moving it just makes the directory
> and all its contents go away all at once.

OK, document this then.

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart
File pkg/watcher/test/utils.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/utils.dart#n...
pkg/watcher/test/utils.dart:257: /// Schedules creating a directory in the
sandbox at [path].
On 2013/11/07 00:46:37, nweiz wrote:
> On 2013/11/06 19:24:04, Bob Nystrom wrote:
> > Can you use the descriptor API instead of adding this?
> 
> I wanted to keep this consistent with the other methods in this file, which
> don't use the descriptor API. I'll add a TODO to switch these tests over to
> using descriptor if you like, but to be honest I don't think it's a great fit.
> These tests usually create only a couple files or directories per test, and
very
> rarely create the sort of complex directory structure that the descriptor API
is
> designed to make easy. I think there's more value in preserving similarity
> between the creation functions and the modification functions in this case
than
> in using the descriptor library for the creation functions.

SGTM. That was my guess behind your thinking here.

[nweiz, 2013-11-07 22:46:27]

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/46843003/diff/1/pkg/watcher/test/directory_wa...
pkg/watcher/test/directory_watcher/shared.dart:193: renameDir("dir/sub", "sub");
On 2013/11/07 18:16:05, Bob Nystrom wrote:
> On 2013/11/07 00:46:37, nweiz wrote:
> > On 2013/11/06 19:24:04, Bob Nystrom wrote:
> > > deleteDir()?
> > 
> > Deleting a directory recursively is different than moving it out of the
> > directory. Recursive deletion usually involves deleting the contents and
then
> > the directory in quick succession, whereas moving it just makes the
directory
> > and all its contents go away all at once.
> 
> OK, document this then.

Done.

[nweiz, 2013-11-07 22:49:32]

Committed patchset #4 manually as r30081 (presubmit successful).