Re-implement directory polling.

Re-implement directory polling.

BUG=https://code.google.com/p/dart/issues/detail?id=12107
R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25746

[Bob Nystrom, 2013-08-01 20:59:18]

I'm hoping this will make it less flaky.

The main changes here are:

1. We process files concurrently while the directory is being listed.
2. If the watcher is unsubscribed, it stops process as soon as the next internal
asynchronous operation completes, regardless of what it is. Instead of waiting
for the while list of files to finish, it can be interrupted at any point.

I think this is generally a change for the better (the old way was a little too
chunky for my taste) and hopefully it will make the tests less flaky.

[nweiz, 2013-08-01 22:52:39]

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

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:61: /// from [_filesToCheck].
"_filesToCheck" -> "_filesToProcess"

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:71: final _filesToProcess = new
Queue<String>();
See how this looks with a FutureGroup instead of a queue, as discussed in
person.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:110: _polledFiles.clear();
Also clear _statuses.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:144: refreshNextFile() {
It feels like this warrants being its own method.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:161: assert(_state !=
_WatchState.UNSUBSCRIBED);
This should be at the top of [refreshNextFile].

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:164: print("done scanning,
watching");
Left over from debugging?

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:196: _events.addError(error,
getAttachedStackTrace(error));
You don't need to manually forward the stack trace.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:207: // If it's modification time
hasn't changed, assume the file is unchanged.
"it's" -> "its"

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:223: if (_state ==
_WatchState.WATCHING) {
Short-circuit here and below.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:260: static const WATCHING = const
_WatchState("watching");
Add some docs about what each state means.

[Bob Nystrom, 2013-08-02 17:26:29]

I've kept the queue because I think it's working well and changing it will be a
lot of work. If it's still flaky after this, I'll look into it more.

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

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:61: /// from [_filesToCheck].
On 2013/08/01 22:52:39, nweiz wrote:
> "_filesToCheck" -> "_filesToProcess"

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:71: final _filesToProcess = new
Queue<String>();
On 2013/08/01 22:52:39, nweiz wrote:
> See how this looks with a FutureGroup instead of a queue, as discussed in
> person.

I started going in that direction but it looks like it will be a lot of work for
unknown benefit. How about we stick with the current approach?

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:110: _polledFiles.clear();
On 2013/08/01 22:52:39, nweiz wrote:
> Also clear _statuses.

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:144: refreshNextFile() {
On 2013/08/01 22:52:39, nweiz wrote:
> It feels like this warrants being its own method.

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:161: assert(_state !=
_WatchState.UNSUBSCRIBED);
On 2013/08/01 22:52:39, nweiz wrote:
> This should be at the top of [refreshNextFile].

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:164: print("done scanning,
watching");
On 2013/08/01 22:52:39, nweiz wrote:
> Left over from debugging?

Yup. Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:196: _events.addError(error,
getAttachedStackTrace(error));
On 2013/08/01 22:52:39, nweiz wrote:
> You don't need to manually forward the stack trace.

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:207: // If it's modification time
hasn't changed, assume the file is unchanged.
On 2013/08/01 22:52:39, nweiz wrote:
> "it's" -> "its"

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:223: if (_state ==
_WatchState.WATCHING) {
On 2013/08/01 22:52:39, nweiz wrote:
> Short-circuit here and below.

Done.

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:260: static const WATCHING = const
_WatchState("watching");
On 2013/08/01 22:52:39, nweiz wrote:
> Add some docs about what each state means.

Done.

[nweiz, 2013-08-02 20:07:52]

LGTM, with one suggestion.

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

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:71: final _filesToProcess = new
Queue<String>();
On 2013/08/02 17:26:29, Bob Nystrom wrote:
> On 2013/08/01 22:52:39, nweiz wrote:
> > See how this looks with a FutureGroup instead of a queue, as discussed in
> > person.
> 
> I started going in that direction but it looks like it will be a lot of work
for
> unknown benefit. How about we stick with the current approach?

SGTM. Can you abstract out the notion of a sequential stream into a class or
method? An API as simple as this should work:

    class SequentialStream<T> {
      SequentialStream(Stream<T> inner, Future callback<T>);
      Future get onDone;
      void cancel();
    }

[Bob Nystrom, 2013-08-02 21:45:11]

Thanks!

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

https://codereview.chromium.org/21628002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher.dart:71: final _filesToProcess = new
Queue<String>();
On 2013/08/02 20:07:52, nweiz wrote:
> On 2013/08/02 17:26:29, Bob Nystrom wrote:
> > On 2013/08/01 22:52:39, nweiz wrote:
> > > See how this looks with a FutureGroup instead of a queue, as discussed in
> > > person.
> > 
> > I started going in that direction but it looks like it will be a lot of work
> for
> > unknown benefit. How about we stick with the current approach?
> 
> SGTM. Can you abstract out the notion of a sequential stream into a class or
> method? An API as simple as this should work:
> 
>     class SequentialStream<T> {
>       SequentialStream(Stream<T> inner, Future callback<T>);
>       Future get onDone;
>       void cancel();
>     }

Done.

[Bob Nystrom, 2013-08-02 22:01:45]

Committed patchset #3 manually as r25746 (presubmit successful).