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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 9310053: Rework Windows process handling. (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

Rework Windows process handling. - Use only one thread to wait for multiple process objects. - Fix issue where reading from a closed pipe would print an error message on stderr. - Remove the need to go to dart before removing a processinfo object from the list. Sometimes you never get there so we have to remove the process info object always. BUG= TEST= Committed: https://code.google.com/p/dart/source/detail?r=3844

Patch Set 1 #

Patch Set 2 : Fix Linux and Mac #

Total comments: 22

Patch Set 3 : Address comments. #

Patch Set 4 : Fix Windows build and add stable binaries. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -120 lines) Patch
M runtime/bin/builtin_natives.cc View 1 chunk +0 lines, -1 line 0 comments Download
M runtime/bin/eventhandler_win.cc View 1 1 chunk +4 lines, -2 lines 0 comments Download
M runtime/bin/process.h View 1 chunk +0 lines, -3 lines 0 comments Download
M runtime/bin/process.cc View 2 chunks +1 line, -9 lines 0 comments Download
M runtime/bin/process_impl.dart View 3 chunks +4 lines, -11 lines 0 comments Download
M runtime/bin/process_linux.cc View 1 2 3 chunks +16 lines, -9 lines 0 comments Download
M runtime/bin/process_macos.cc View 1 2 3 chunks +16 lines, -9 lines 0 comments Download
M runtime/bin/process_win.cc View 1 2 3 7 chunks +312 lines, -79 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-02-02 12:07:13 UTC) #1
Søren Gjesse
lgtm http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_linux.cc File runtime/bin/process_linux.cc (right): http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_linux.cc#newcode201 runtime/bin/process_linux.cc:201: // WriteFile fails with a broken pipe error. ...
8 years, 10 months ago (2012-02-02 12:26:07 UTC) #2
Mads Ager (google)
8 years, 10 months ago (2012-02-02 13:47:45 UTC) #3 
http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_linux.cc...
runtime/bin/process_linux.cc:201: // WriteFile fails with a broken pipe error.
Other errors
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> WriteFile -> WriteToBlocking (or just write)

Done.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_macos.cc
File runtime/bin/process_macos.cc (right):

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_macos.cc...
runtime/bin/process_macos.cc:200: // WriteFile fails with a broken pipe error.
Other errors
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:83: static DWORD LookupProcessByHandle(HANDLE handle,
DWORD* pid, HANDLE* pipe) {
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> bool return type.

Good catch. Thanks.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:144: if (process_added_event_ == 0) {
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> INVALID_HANDLE_VALUE instead of 0.

Done.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:146: if (process_added_event_ == NULL) {
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> INVALID_HANDLE_VALUE instead of NULL.

Not on this one. Strange, I agree, but that's what the documentation says: 

http://msdn.microsoft.com/en-us/library/windows/desktop/ms682396(v=vs.85).aspx

"If the function fails, the return value is NULL"

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:170: HANDLE ProcessInfoList::process_added_event_ =
0;
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> INVALID_HANDLE_VALUE instead of 0.

Done.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:193: if (terminate_event_ == NULL) {
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> INVALID_HANDLE_VALUE instead of NULL.

Not for this one according to CreateEvent documentation.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:203: new dart::Thread(ExitCodeHandlerEntry,
reinterpret_cast<uword>(events));
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> int result = dart::Thread::Start(ExitCodeHandlerEntry,
> reinterpret_cast<uword>(events));
> if (result != 0) {
>   FATAL1("Failed to start exit code handler thread %d", result);
> }
> 
>  - you'll see when rebasing.

Done.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:210: static void TerminateExitCodeThread() {
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> Maybe add a comment for this function that it actually waits for the thread to
> terminate.

Yes, that comment is in the process.h file. :)

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:248: HANDLE* handles;
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> How about just declaring this as
> 
> HANDLE handles[MAXIMUM_WAIT_OBJECTS]
> 
> then you don't need to allocate/deallocate.

Done. It needs to be dealt with in GetHandleArray as well so we do not get
out-of-bounds writes to the array.

http://codereview.chromium.org/9310053/diff/1009/runtime/bin/process_win.cc#n...
runtime/bin/process_win.cc:334: HANDLE ExitCodeHandler::terminate_event_ = 0;
On 2012/02/02 12:26:07, Søren Gjesse wrote:
> INVALID_HANDLE_VALUE instead of 0.

Done.

Powered by Google App Engine
This is Rietveld 408576698