File watching package.

pkg/watcher/lib/src/directory_watcher.dart

+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library watcher.directory_watcher;
+
+import 'dart:async';
+import 'dart:io';
+
+import 'package:crypto/crypto.dart';
+
+import 'change_type.dart';
+import 'stat.dart';
+import 'watch_event.dart';
+
+/// Watches the contents of a directory and emits [WatchEvents] when something

[nweiz, 2013/07/11 00:25:01]

"[WatchEvent]s"

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+/// in the directory has changed.
+class DirectoryWatcher {
+  /// The directory whose contents are being monitored.
+  final String directory;
+
+  /// The [Stream] of file modification events that have occurred to files in

[nweiz, 2013/07/11 00:25:01]

"file modification events" -> "events"

Also, mention that this is a broadcast stream, since the type doesn't
communicate that.

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+  /// [directory].
+  ///
+  /// Changes will only be monitored while this stream has subscribers. Any
+  /// file changes that occur during periods when there are no subscribers
+  /// will not be reported the next time a subscriber is added.
+  Stream<WatchEvent> get events => _events.stream;
+

[nweiz, 2013/07/11 00:25:01]

Style nit: I'd get rid of this newline to indicate that [events] and its
controller are paired.

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+  StreamController<WatchEvent> _events;

[Siggi Cherem (dart-lang), 2013/07/10 22:53:53]

change to final?

[Bob Nystrom, 2013/07/10 23:04:39]

Wish I could. :(

The only way to set onListen and onCancel on a StreamController is in its
constructor, and the functions I'm providing there need access to the
DirectoryWatcher. This means it has to be called in the constructor *body*,
which is too late for final.

[nweiz, 2013/07/11 00:25:01]

I'd call this _eventsController.

[Bob Nystrom, 2013/07/11 18:33:20]

Since the stream itself isn't stored, I think that's a little painfully verbose.
I'll leave it as-is if that's OK.

+
+  _WatchState _state = _WatchState.notWatching;
+
+  /// The previous status of the files in the directory. Used to tell which
+  /// files have been modified.

[nweiz, 2013/07/11 00:25:01]

Style nit: first sentence should be its own paragraph.

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+  final _statuses = new Map<String, _FileStatus>();
+
+  /// Creates a new [DirectoryWatcher] monitoring [directory].
+  DirectoryWatcher(this.directory) {
+    _events = new StreamController<WatchEvent>.broadcast(onListen: () {
+      _state = _state.listen(this);
+    }, onCancel: () {
+      _state = _state.cancel(this);
+    });
+  }
+
+  /// Starts the asynchronous polling process.
+  ///
+  /// Scans the contents of the directory and compares the results to the
+  /// previous scan. Loops to continue monitoring as long as there are
+  /// subscribers to the [events] stream.
+  Future _watch() {
+    var files = new Set<String>();
+
+    var stream = new Directory(directory).list(
+        recursive: true, followLinks: true);

[nweiz, 2013/07/11 00:25:01]

I'd much rather we use pub's listDirectory here, either adapted to be
asynchronous or just used synchronously. [Directory.list] still screws up edge
cases when symlinks are involved.

[Bob Nystrom, 2013/07/11 18:33:20]

I think it's important for watcher to use a stream API for this, and modifying
pub's listDirectory() to be stream-based would be a big chunk of work. dart:io's
handling of symlinks is much better than it used to be, and they're responding
to bugs much faster. I'd like to try just using the vanilla API and push on them
when/if we run into problems.

[nweiz, 2013/07/11 22:29:00]

They've marked an important bug as wontfix:
https://code.google.com/p/dart/issues/detail?id=4794. Recursive symlinks are an
edge case, sure, but the [Directory.list] behavior in that edge case is bad
enough that it's likely to cause serious and subtle bugs for anyone who runs
into it.

[Bob Nystrom, 2013/07/12 00:31:09]

I think you marked that bug WontFix. I changed this to not follow symlinks which
should avoid that issue.

[nweiz, 2013/07/12 01:04:28]

Soren incorrectly marked it as fixed. I changed the status to accurately reflect
the stated resolution.
Are we okay with barback (and other potential users of this library) not
supporting symlinked directories?

[Bob Nystrom, 2013/07/12 17:46:16]

Yes, for now I'm OK with not supported symlinked directories (or directories
containing symlinks). We can always expand this functionality over time.

+
+    var futures = [];
+    return stream.forEach((entity) {
+      if (entity is File) {
+        files.add(entity.path);
+        futures.add(_refreshFile(entity.path));
+      }
+    }).then((_) {
+      // Once the listing is done, make sure to wait until each file is also
+      // done.
+      return Future.wait(futures);
+    }).then((_) {

[nweiz, 2013/07/11 00:25:01]

I think the following would be a little cleaner here (gets rid of the
weirdly-scoped [futures] variable):

    return stream.map((entity) {
      if (entity is! File) return new Future.value();
      files.add(entity.path);
      return _refreshFile(entity.path);
    }).then((futures) {
      return Future.wait(futures);
    })

[Bob Nystrom, 2013/07/11 18:33:20]

Had to stick a toList() in there, but this is a big improvement. Done.

+      var removedFiles = _statuses.keys.toSet().difference(files);
+      for (var removed in removedFiles) {
+        if (_state.shouldNotify) {

[nweiz, 2013/07/11 00:25:01]

It's a little weird that this is a flag, but you just explicitly check that
_state is watching below.

[Bob Nystrom, 2013/07/11 18:33:20]

Changed to check shouldNotify below.

+          _events.add(new WatchEvent(ChangeType.REMOVE, removed));
+        }
+        _statuses.remove(removed);
+      }
+
+      _state = _state.finish(this);
+
+      // If the new state isn't watching, just stop.
+      if (!_state.shouldWatch) return;
+
+      // If we're in the "watching" state, add a bit of delay before restarting
+      // just so that we don't whale on the file system.
+      // TODO(rnystrom): Tune this and/or make it tunable?
+      if (_state == _WatchState.watching) {
+        return new Future.delayed(new Duration(seconds: 1));
+      }
+
+      // Otherwise, loop.
+      return _watch();

[nweiz, 2013/07/11 00:25:01]

Shouldn't this just be "return;"? Otherwise, wouldn't [watch] get called twice?

If you change this to "return", you can get rid of the [shouldWatch] check
above.

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+    }).then((_) {
+      // Make sure we haven't transitioned to a non-watching state during the
+      // delay.
+      if (_state.shouldWatch) _watch();
+    });
+  }
+
+  /// Compares the current state of the file at [path] to the state it was in
+  /// the last time it was scanned.
+  Future _refreshFile(String path) {
+    return getModificationTime(path).then((modified) {
+      var lastStatus = _statuses[path];
+
+      // If it's modification time hasn't changed, assume the file is unchanged.
+      if (lastStatus != null && lastStatus.modified == modified) {
+        return false;

[nweiz, 2013/07/11 00:25:01]

Is this return value used anywhere?

[Bob Nystrom, 2013/07/11 18:33:20]

Oops. Not anymore. Used to have the later future stuff popped out a level.
Removed.

+      }
+
+      return _hashFile(path).then((hash) {
+        var status = new _FileStatus(modified, hash);
+        _statuses[path] = status;
+
+        if (!_state.shouldNotify) return;

[nweiz, 2013/07/11 00:25:01]

Fold this into the following "if", or vice versa.

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+
+        // Only notify if the file contents are changed.
+        if (lastStatus == null || !_sameHash(lastStatus.hash, hash)) {

[Siggi Cherem (dart-lang), 2013/07/10 22:53:53]

consider making this a configuration option - some applications might want to be
notified on every change, even if the contents are identical. 

[Bob Nystrom, 2013/07/10 23:04:39]

I expect this to get more configurable over time, but I figured for the first
patch I'd start simple. I can always add options, it's hard to take them away.

+          var change = lastStatus == null ? ChangeType.ADD : ChangeType.MODIFY;
+          _events.add(new WatchEvent(change, path));
+        }
+      });
+    });
+  }
+
+  /// Calculates the SHA-1 hash of the file at [path].
+  Future<List<int>> _hashFile(String path) {
+    return new File(path).readAsBytes().then((bytes) {
+      var sha1 = new SHA1();
+      sha1.add(bytes);
+      return sha1.close();
+    });
+  }
+
+  /// Returns `true` if [a] and [b] are the same hash value, i.e. the same
+  /// series of byte values.
+  bool _sameHash(List<int> a, List<int> b) {

[Siggi Cherem (dart-lang), 2013/07/10 22:53:53]

alternatively call CrytoUtils.bytesToHex and compare them as strings?

[Bob Nystrom, 2013/07/10 23:04:39]

Hmm, is that faster? Seems like a roundabout way to do it.

+    // Hashes should always be the same size.
+    assert(a.length == b.length);
+
+    for (var i = 0; i < a.length; i++) {
+      if (a[i] != b[i]) return false;
+    }
+
+    return true;
+  }
+}
+
+/// An "event" that is sent to the [_WatchState] FSM to trigger state
+/// transitions.
+typedef _WatchState _WatchStateEvent(DirectoryWatcher watcher);
+
+/// The different states that the watcher can be in and the transitions between
+/// them.
+///
+/// This class defines a finite state machine for keeping track of what the
+/// asynchronous file polling is doing. Each instance of this is a state in the
+/// machine and its [listen], [cancel], and [finish] fields define the state
+/// transitions when those events occur.
+class _WatchState {
+  /// The watcher has no subscribers.
+  static final notWatching = new _WatchState(
+      listen: (watcher) {
+    watcher._watch();

[nweiz, 2013/07/11 00:25:01]

If you move this into DirectoryWatcher, then _WatchState won't need to access
any private members of DirectoryWatcher, so you can put it into its own library.

[Bob Nystrom, 2013/07/11 18:33:20]

You mean move the state instances?
_WatchState is deeply tied to DirectoryWatcher, so I actually like that it's in
the same library. I don't think we need strict one-class-per-library style. If
this was C++, they'd be friend classes.

[nweiz, 2013/07/11 22:29:00]

No, I mean the call to "watcher._watch" in particular.
I don't think the one-class-per-library style should be strict, but I do think
it generally makes code easier to read, both by limiting the size of files and
encouraging a use of private names that communicates more information.

As I mentioned above, this line is the only one in the class where WatchState
refers to a private member of DirectoryWatcher. In fact, it's the only one where
WatchState refers to DirectoryWatcher at all.

[Bob Nystrom, 2013/07/12 00:31:09]

Agreed on both accounts. In this case, I think having both classes in the
library makes sense: they are intimately tied. _WatchState is completely an
implementation detail of DirectoryWatcher's current state. If Dart had nested
classes, I would probably use one for this.

[nweiz, 2013/07/12 01:04:28]

Okay.

+    return _WatchState.scanning;
+  });
+
+  /// The watcher has subscribers and is scanning for pre-existing files.
+  static final scanning = new _WatchState(
+      cancel: (_) => _WatchState.cancelling,
+      finish: (_) => _WatchState.watching,
+      shouldWatch: true);
+
+  /// The watcher was unsubscribed while polling and we're waiting for the poll
+  /// to finish.
+  static final cancelling = new _WatchState(
+      listen: (_) => _WatchState.scanning,
+      finish: (_) => _WatchState.notWatching);
+
+  /// The watcher has subscribers, we have scanned for pre-existing files and
+  /// now we're waiting for changes to come in.

[nweiz, 2013/07/11 00:25:01]

"waiting for changes to come in" -> "polling for changes"

[Bob Nystrom, 2013/07/11 18:33:20]

Done.

+  static final watching = new _WatchState(
+      cancel: (_) => _WatchState.cancelling,
+      finish: (_) => _WatchState.watching,
+      shouldWatch: true, shouldNotify: true);
+
+  /// Asserts that an event is not expected for some state.
+  static _WatchState _badState(DirectoryWatcher watcher) {
+    // Should not receive this event in this state.
+    assert(false);

[nweiz, 2013/07/11 00:25:01]

If it's possible, it would be nice to provide some metadata here about what
event was received and which state received it.

[Bob Nystrom, 2013/07/11 18:33:20]

I just went ahead and deleted this. It only exist(s|ed) to trap programmatic
errors in the state machine, but it's so simple that I don't think it's worth
it.

Now, you'll get a NPE if you try to send an event (listen, cancel, finish) on a
state that doesn't expect that.

+  }
+
+  /// Called when the first subscriber to the watcher has been added.
+  final _WatchStateEvent listen;
+
+  /// Called when all subscriptions on the watcher have been cancelled.
+  final _WatchStateEvent cancel;
+
+  /// Called when a poll loop has finished.
+  final _WatchStateEvent finish;
+
+  /// If the directory watcher should be watching the file system while in
+  /// this state.
+  final bool shouldWatch;
+
+  /// `true` if a change event should be sent for a file modification while
+  /// in this state.
+  final bool shouldNotify;
+
+  _WatchState({
+    _WatchStateEvent listen,
+    _WatchStateEvent cancel,
+    _WatchStateEvent finish,
+    this.shouldWatch: false,
+    this.shouldNotify: false})
+      : listen = listen != null ? listen : _badState,
+        cancel = cancel != null ? cancel : _badState,
+        finish = finish != null ? finish : _badState;
+}
+
+class _FileStatus {
+  /// The last time the file was modified.
+  DateTime modified;
+
+  /// The SHA-1 hash of the contents of the file.
+  List<int> hash;
+
+  _FileStatus(this.modified, this.hash);
+}