Add web socket command to stop serving a directory.

Add web socket command to stop serving a directory.

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

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

[Bob Nystrom, 2014-03-18 18:24:09]

This lets you unbind a port that's being served. There are still two known
issues here:

1. You get redundant overlapping watchers if you bind overlapping directories.
This is good in that unbinding one of those directories doesn't incorrectly
cause the overlapping part to stop being watched, but it means we'll do
redundant building sometimes.

2. If you unserve the directory whose port you are also using for the web socket
connection, it will close that connection. :)

To handle the latter, we'll eventually have a dedicated port for the web socket
API (and admin UI): https://code.google.com/p/dart/issues/detail?id=16957

But I figured this was a good point to get it checked in and let Keerti take a
look.

[nweiz, 2014-03-18 19:33:54]

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:172: Future<String>
unserveDirectory(String rootDirectory) {
If my websocket change lands before this, this should return a [Future<Uri>].

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:341: /// Returns
the subscription on the watcher, or `null` if none was created.
"on" -> "to"

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:353: return
_watchDirectorySources(package, dir);
I know you didn't change this in this CL, but it just occurred to me that this
logic is slightly wrong. In "pub serve", it's possible to miss a file if it's
created between [_listDirectorySources] listing it and the watcher beginning to
run. If we're starting a watcher, we should watch the sources before we update
them.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:373: return
!_directories.keys.any((dir) => path.isWithin(dir, relative));
This is O(n*m) for servers and files. Add a TODO to do something more clever
here.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:390: /// packages.
Only assets whose relative path matches that will be included
Does "relative path" mean "relative to the current working directory" or
"relative to the root of [package]"? Clarify that here.

If the latter, I think the [isWithin] call in [_removeDirectorySources] is
wrong.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:489: class
_BuildDirectory {
Nit: this file is pretty long already, I'd move [_BuildDirectory] to its own
library.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart (right):

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:302: /// The command
name is "unserveDirectory" and it takes a "path" key for the
Mention that "path" is a native OS path that's relative to the root of the
entrypoint package.

[Bob Nystrom, 2014-03-18 21:24:57]

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:172: Future<String>
unserveDirectory(String rootDirectory) {
On 2014/03/18 19:33:54, nweiz wrote:
> If my websocket change lands before this, this should return a [Future<Uri>].

Not if mine lands first! :)

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:341: /// Returns
the subscription on the watcher, or `null` if none was created.
On 2014/03/18 19:33:54, nweiz wrote:
> "on" -> "to"

Done.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:353: return
_watchDirectorySources(package, dir);
On 2014/03/18 19:33:54, nweiz wrote:
> I know you didn't change this in this CL, but it just occurred to me that this
> logic is slightly wrong. In "pub serve", it's possible to miss a file if it's
> created between [_listDirectorySources] listing it and the watcher beginning
to
> run. If we're starting a watcher, we should watch the sources before we update
> them.

Done.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:373: return
!_directories.keys.any((dir) => path.isWithin(dir, relative));
On 2014/03/18 19:33:54, nweiz wrote:
> This is O(n*m) for servers and files. Add a TODO to do something more clever
> here.

Done. I figured the number of servers is small so this was a simple solution
that didn't require a bunch of special casing.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:390: /// packages.
Only assets whose relative path matches that will be included
On 2014/03/18 19:33:54, nweiz wrote:
> Does "relative path" mean "relative to the current working directory" or
> "relative to the root of [package]"? Clarify that here.

It's within [package]. Done.

> 
> If the latter, I think the [isWithin] call in [_removeDirectorySources] is
> wrong.

I think it's correct with the assumption that _removeDirectorySources is only
called on the entrypoint package. Made that explicit by removing the package
parameter from that. (We only serve out of the entrypoint package, and we only
support unserving out of it.)

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:489: class
_BuildDirectory {
On 2014/03/18 19:33:54, nweiz wrote:
> Nit: this file is pretty long already, I'd move [_BuildDirectory] to its own
> library.

Done.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart (right):

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:302: /// The command
name is "unserveDirectory" and it takes a "path" key for the
On 2014/03/18 19:33:54, nweiz wrote:
> Mention that "path" is a native OS path that's relative to the root of the
> entrypoint package.

Done.

[nweiz, 2014-03-18 22:08:30]

lgtm

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:373: return
!_directories.keys.any((dir) => path.isWithin(dir, relative));
On 2014/03/18 21:24:58, Bob Nystrom wrote:
> On 2014/03/18 19:33:54, nweiz wrote:
> > This is O(n*m) for servers and files. Add a TODO to do something more clever
> > here.
> 
> Done. I figured the number of servers is small so this was a simple solution
> that didn't require a bunch of special casing.

Hopefully fixing the multiple-serve issue will also produce something nice here.

https://codereview.chromium.org/203623004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:390: /// packages.
Only assets whose relative path matches that will be included
On 2014/03/18 21:24:58, Bob Nystrom wrote:
> On 2014/03/18 19:33:54, nweiz wrote:
> > Does "relative path" mean "relative to the current working directory" or
> > "relative to the root of [package]"? Clarify that here.
> 
> It's within [package]. Done.
> 
> > 
> > If the latter, I think the [isWithin] call in [_removeDirectorySources] is
> > wrong.
> 
> I think it's correct with the assumption that _removeDirectorySources is only
> called on the entrypoint package. Made that explicit by removing the package
> parameter from that. (We only serve out of the entrypoint package, and we only
> support unserving out of it.)

SGTM

https://codereview.chromium.org/203623004/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/203623004/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/server.dart:156: })
Leftover from debugging?

[Bob Nystrom, 2014-03-18 22:13:47]

Thanks!

https://codereview.chromium.org/203623004/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/203623004/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/server.dart:156: })
On 2014/03/18 22:08:30, nweiz wrote:
> Leftover from debugging?

Not sure what that is, but I think I didn't touch this. Either way, fixed.

[Bob Nystrom, 2014-03-18 22:31:05]

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