Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(532)

Issue 9293030: Move actual work out of the SIGCHLD signal handler for the dart:io (Closed)

Created:
8 years, 10 months ago by Mads Ager (google)
Modified:
8 years, 10 months ago
Reviewers:
Søren Gjesse
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Move actual work out of the SIGCHLD signal handler for the dart:io process library. Use the self-pipe trick to signal a dedicated thread when a SIGCHLD signal is received. The dedicated thread will perform the actual work of extracting the exit code and posting it back to Dart. R=sgjesse@google.com BUG= TEST= Committed: https://code.google.com/p/dart/source/detail?r=3711

Patch Set 1 #

Patch Set 2 : Fix typo #

Total comments: 4

Patch Set 3 : Update stable test binaries #

Unified diffs Side-by-side diffs Delta from patch set Stats (+369 lines, -129 lines) Patch
M runtime/bin/process.dart View 3 chunks +6 lines, -6 lines 0 comments Download
M runtime/bin/process_linux.cc View 1 7 chunks +183 lines, -63 lines 0 comments Download
M runtime/bin/process_macos.cc View 1 7 chunks +183 lines, -63 lines 0 comments Download
M tools/testing/bin/linux/dart View 0 chunks +-1 lines, --1 lines 0 comments Download
M tools/testing/bin/macos/dart View 0 chunks +-1 lines, --1 lines 0 comments Download
M tools/testing/bin/windows/dart.exe View 0 chunks +-1 lines, --1 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Mads Ager (google)
8 years, 10 months ago (2012-01-31 07:52:26 UTC) #1
Søren Gjesse
LGTM! https://chromiumcodereview.appspot.com/9293030/diff/2001/runtime/bin/process_linux.cc File runtime/bin/process_linux.cc (right): https://chromiumcodereview.appspot.com/9293030/diff/2001/runtime/bin/process_linux.cc#newcode40 runtime/bin/process_linux.cc:40: // Singly-linked list of ProcessInfo objects for all ...
8 years, 10 months ago (2012-01-31 08:19:18 UTC) #2
Mads Ager (google)
8 years, 10 months ago (2012-01-31 08:37:47 UTC) #3
https://chromiumcodereview.appspot.com/9293030/diff/2001/runtime/bin/process_...
File runtime/bin/process_linux.cc (right):

https://chromiumcodereview.appspot.com/9293030/diff/2001/runtime/bin/process_...
runtime/bin/process_linux.cc:40: // Singly-linked list of ProcessInfo objects
for all active processes
On 2012/01/31 08:19:18, Søren Gjesse wrote:
> We could consider changing this list to a hash map.

Agreed, we could. This is so simple that I would like to keep it for now. Let's
see if we can make these methods show up on a profile. :-)

https://chromiumcodereview.appspot.com/9293030/diff/2001/runtime/bin/process_...
runtime/bin/process_linux.cc:187: ssize_t read_bytes =
FDUtils::ReadFromBlocking(pollfds.fd, &data, 1);
On 2012/01/31 08:19:18, Søren Gjesse wrote:
> Should there be a way to gracefully shutdown this thread, e.g writing 255?
> 
> In main we could then have a call to ExitCodeHandler::Shutdown().

As discussed offline, we need to do this both for this new thread and for the
event-handler thread on isolate shutdown. Let me look into that in a separate
changelist.

Powered by Google App Engine
This is Rietveld 408576698