Signal handling.

Signal handling.

BUG=
R=sgjesse@google.com

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

[Anders Johnsen, 2013-12-17 15:15:51]

Early CL for signal handling. This is sent out to discuss API and initial
implementation approach.

Including is working (but not well tested) Linux and Windows versions.

[Søren Gjesse, 2013-12-18 08:07:14]

I think the API and implementation approach looks fine.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:640: SIGTERM
Shouldn't the SIGUSR1 and SIGUSR2 be in this list as well?

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:674: static SignalInfo* signal_handlers[32] = {
NULL };
Wouldn't it be OK to have the signal in the SignalInfo class, and just go
through all of them for all signals?

Otherwise add a constant kMaxSignal and check on that when adding.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:681: int value = 0;
Can we loose a signal here if the non-blocking write returns 0. Maybe the pipe
is full as for some reason the eventhandler is not fast enough to read from the
pipe. Probably very unlikely.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:688: intptr_t Process::SetSignalHandler(intptr_t
signal) {
Check that the signal is actually in the kSignals array.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:724: while (handler != NULL) {
Why this loop? Seems that port is always equals to Dart_GetMainPortId(), and if
not this is an infinite loop.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_patc...
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_patc...
runtime/bin/process_patch.dart:135: signal != ProcessSignal.SIGWINCH) {
Different set for Windows?

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_patc...
runtime/bin/process_patch.dart:137: "It's not allowed to listen for $signal
signals.");
"Listening for signal $signal is not supported"

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_win....
runtime/bin/process_win.cc:928: if (signal == -1) return -1;
Throw here?

Dart_ThrowException(DartUtils::NewDartArgumentError("Illegal argument to
SetSignalHandler"));

The Dart code should check this anyway, but is an unsupported value gets through
I think it is better to throw.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_win....
runtime/bin/process_win.cc:929: // Generate a unique pipe name for the named
pipe.
Please factor out this named pipe name generation to avoid code duplication.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/signal_block...
File runtime/bin/signal_blocker.h (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/signal_block...
runtime/bin/signal_blocker.h:37: int r = pthread_sigmask(SIG_BLOCK,
&signal_mask, &old);
You will probably need USE(r) for release mode.

[Anders Johnsen, 2013-12-18 21:20:29]

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:640: SIGTERM
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Shouldn't the SIGUSR1 and SIGUSR2 be in this list as well?

Done.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:674: static SignalInfo* signal_handlers[32] = {
NULL };
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Wouldn't it be OK to have the signal in the SignalInfo class, and just go
> through all of them for all signals?
> 
> Otherwise add a constant kMaxSignal and check on that when adding.

Done.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:681: int value = 0;
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Can we loose a signal here if the non-blocking write returns 0. Maybe the pipe
> is full as for some reason the eventhandler is not fast enough to read from
the
> pipe. Probably very unlikely.

This write is blocking.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:688: intptr_t Process::SetSignalHandler(intptr_t
signal) {
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Check that the signal is actually in the kSignals array.

Done.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:724: while (handler != NULL) {
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Why this loop? Seems that port is always equals to Dart_GetMainPortId(), and
if
> not this is an infinite loop.

This is a way to map it to different isolates. We should only remove for the
right isolate. But there was a bug, now fixed.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_patc...
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_patc...
runtime/bin/process_patch.dart:135: signal != ProcessSignal.SIGWINCH) {
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Different set for Windows?

Done.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_patc...
runtime/bin/process_patch.dart:137: "It's not allowed to listen for $signal
signals.");
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> "Listening for signal $signal is not supported"

Done.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_win....
runtime/bin/process_win.cc:928: if (signal == -1) return -1;
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Throw here?
> 
> Dart_ThrowException(DartUtils::NewDartArgumentError("Illegal argument to
> SetSignalHandler"));
> 
> The Dart code should check this anyway, but is an unsupported value gets
through
> I think it is better to throw.

Returning -1 will throw as well (in process.cc). While it's not a 'correct'
exception, it's fine as it's not supposed to happen, but will guide us to the
right fix.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/process_win....
runtime/bin/process_win.cc:929: // Generate a unique pipe name for the named
pipe.
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> Please factor out this named pipe name generation to avoid code duplication.

Done.

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/signal_block...
File runtime/bin/signal_blocker.h (right):

https://codereview.chromium.org/108003009/diff/60001/runtime/bin/signal_block...
runtime/bin/signal_blocker.h:37: int r = pthread_sigmask(SIG_BLOCK,
&signal_mask, &old);
On 2013/12/18 08:07:14, Søren Gjesse wrote:
> You will probably need USE(r) for release mode.

Done.

[Søren Gjesse, 2013-12-19 08:51:24]

Fine for adding the remaining platforms.

https://codereview.chromium.org/108003009/diff/80001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/108003009/diff/80001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:646: class SignalInfo {
This class could should go into the platform independent file.

https://codereview.chromium.org/108003009/diff/80001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:680: Dart_Port port_;
Please add some documentation to the fields. Especially say that port_
identifies the isolate.

https://codereview.chromium.org/108003009/diff/80001/tests/standalone/io/sign...
File tests/standalone/io/signals_test.dart (right):

https://codereview.chromium.org/108003009/diff/80001/tests/standalone/io/sign...
tests/standalone/io/signals_test.dart:27: int count = out.where((c) => c ==
10).length;
Please change 10 to '\n'.charCodeAt(0) (or make a constant NL with the value
10).

Please add a comment "Send as many signals as 'ready\n' received on stdout.

[Anders Johnsen, 2013-12-19 10:54:17]

PTAL

https://codereview.chromium.org/108003009/diff/80001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/108003009/diff/80001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:646: class SignalInfo {
On 2013/12/19 08:51:24, Søren Gjesse wrote:
> This class could should go into the platform independent file.

Done.

https://codereview.chromium.org/108003009/diff/80001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:680: Dart_Port port_;
On 2013/12/19 08:51:24, Søren Gjesse wrote:
> Please add some documentation to the fields. Especially say that port_
> identifies the isolate.

Done.

https://codereview.chromium.org/108003009/diff/80001/tests/standalone/io/sign...
File tests/standalone/io/signals_test.dart (right):

https://codereview.chromium.org/108003009/diff/80001/tests/standalone/io/sign...
tests/standalone/io/signals_test.dart:27: int count = out.where((c) => c ==
10).length;
On 2013/12/19 08:51:24, Søren Gjesse wrote:
> Please change 10 to '\n'.charCodeAt(0) (or make a constant NL with the value
> 10).
> 
> Please add a comment "Send as many signals as 'ready\n' received on stdout.

Done.

[Søren Gjesse, 2013-12-19 12:47:46]

lgtm

[Anders Johnsen, 2013-12-19 13:08:39]

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