SIGQUIT toggles VM service HTTP server

SIGQUIT toggles VM service HTTP server

R=ajohnsen@google.com

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

[Ivan Posva, 2014-04-14 16:12:57]

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/server...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/server...
runtime/bin/vmservice/server.dart:139: _displayStatusMessages = true;
Shouldn't the fact whether you are forced to shutdown be a parameter?

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
runtime/bin/vmservice/vmservice_io.dart:25: void _onSignal(ProcessSignal signal)
{
_onUSR1Signal or _onVMServiceSignal

[turnidge, 2014-04-14 16:51:46]

https://codereview.chromium.org/237113002/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/main.cc#newcode70
runtime/bin/main.cc:70: static int vm_service_server_port = 0;
Comment here on what a 0 port means.

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
runtime/bin/vmservice/vmservice_io.dart:38:
ProcessSignal.SIGUSR1.watch().listen(_onSignal);
If a signal is processed immediately at this point, could the server be started
twice?

Does it make sense to move the listen() call to after the end of this function,
after the server has started?

[Cutch, 2014-04-15 17:42:07]

https://codereview.chromium.org/237113002/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/main.cc#newcode70
runtime/bin/main.cc:70: static int vm_service_server_port = 0;
On 2014/04/14 16:51:46, turnidge wrote:
> Comment here on what a 0 port means.

I had to change this code. I put the comment about the 0 port in
vmservice_impl.cc.

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/server...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/server...
runtime/bin/vmservice/server.dart:139: _displayStatusMessages = true;
On 2014/04/14 16:12:58, Ivan Posva wrote:
> Shouldn't the fact whether you are forced to shutdown be a parameter?

Done.

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
runtime/bin/vmservice/vmservice_io.dart:25: void _onSignal(ProcessSignal signal)
{
On 2014/04/14 16:12:58, Ivan Posva wrote:
> _onUSR1Signal or _onVMServiceSignal

Done.

https://codereview.chromium.org/237113002/diff/1/runtime/bin/vmservice/vmserv...
runtime/bin/vmservice/vmservice_io.dart:38:
ProcessSignal.SIGUSR1.watch().listen(_onSignal);
On 2014/04/14 16:51:46, turnidge wrote:
> If a signal is processed immediately at this point, could the server be
started
> twice?
> 
> Does it make sense to move the listen() call to after the end of this
function,
> after the server has started?

dart:io signals aren't delivered like regular signals are delivered, signals
received by the process are queued up and shared across isolates. But, I've
moved the signal listening code to be after the startup. I've also modified the
server to ignore startup/shutdown requests while transitioning between the
shutdown and startup states.

[Cutch, 2014-05-22 17:24:48]

+ ajohnsen to review I/O server startup and shutdown.

[Anders Johnsen, 2014-05-23 06:06:35]

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:83: static const SLASH_PATH = '/index.html';
Maybe ROOT_REDIRECT_PATH is more descriptive.

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:128: if (_server != null) {
_server is not non-null until the bind is complete. We should introduce a new
field to guard this, so we don't bind twice, on a very fast series of signals.

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:157: _displayMessages = _displayMessages ||
forced;
Do we really want to override the _displayMessages field? Perhaps just a local
variable would be sufficient.

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:162: _subscription.cancel();
No need to cancel subscription when closing the server. I don't think we can do
this without the subscription at all.

[Cutch, 2014-06-16 22:21:12]

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:83: static const SLASH_PATH = '/index.html';
On 2014/05/23 06:06:36, Anders Johnsen wrote:
> Maybe ROOT_REDIRECT_PATH is more descriptive.

Done.

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:128: if (_server != null) {
On 2014/05/23 06:06:36, Anders Johnsen wrote:
> _server is not non-null until the bind is complete. We should introduce a new
> field to guard this, so we don't bind twice, on a very fast series of signals.

This is guarded against in the SIGQUIT handler.

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:157: _displayMessages = _displayMessages ||
forced;
On 2014/05/23 06:06:36, Anders Johnsen wrote:
> Do we really want to override the _displayMessages field? Perhaps just a local
> variable would be sufficient.

We need to track the state across multiple calls.

https://codereview.chromium.org/237113002/diff/60001/runtime/bin/vmservice/se...
runtime/bin/vmservice/server.dart:162: _subscription.cancel();
On 2014/05/23 06:06:36, Anders Johnsen wrote:
> No need to cancel subscription when closing the server. I don't think we can
do
> this without the subscription at all.

Done.

[Cutch, 2014-06-16 22:28:10]

PTAL.

[Anders Johnsen, 2014-06-17 08:28:36]

lgtm

[Cutch, 2014-06-17 14:26:57]

Committed patchset #6 manually as r37396 (presubmit successful).