Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478473003:
[d8] cleanly force exit in d8 on windows. (Closed)
Description[d8] cleanly force exit in d8 on windows.
Using _exit on windows may cause race conditions in threads.
BUG=chromium:603131
Committed: https://crrev.com/639f6f1774cd73d392284fa090ce012aebcaa363
Cr-Commit-Position: refs/heads/master@{#40789}
Patch Set 1 #
Total comments: 5
Patch Set 2 : wait for termination #Patch Set 3 : fix #Patch Set 4 : . #
Messages
Total messages: 25 (16 generated)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yangguo@chromium.org changed reviewers: + brucedawson@chromium.org, machenbach@chromium.org
lgtm with comments: https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc File src/d8-windows.cc (right): https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc#newcode14 src/d8-windows.cc:14: // Use _exit instead of exit to avoid races between isolate This comment doesn't apply anymore. https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc#newcode18 src/d8-windows.cc:18: TerminateProcess(GetCurrentProcess(), exit_code); Do we have to wait for the process to exit? Similar to https://cs.chromium.org/chromium/src/base/process/process_win.cc?q=TerminateP...
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc File src/d8-windows.cc (right): https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc#newcode14 src/d8-windows.cc:14: // Use _exit instead of exit to avoid races between isolate On 2016/11/04 10:40:44, machenbach (slow) wrote: > This comment doesn't apply anymore. Done. https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc#newcode18 src/d8-windows.cc:18: TerminateProcess(GetCurrentProcess(), exit_code); On 2016/11/04 10:40:44, machenbach (slow) wrote: > Do we have to wait for the process to exit? Similar to > https://cs.chromium.org/chromium/src/base/process/process_win.cc?q=TerminateP... good point.
lgtm, with one caveat about a probably unnecessary wait. https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc File src/d8-windows.cc (right): https://codereview.chromium.org/2478473003/diff/1/src/d8-windows.cc#newcode18 src/d8-windows.cc:18: TerminateProcess(GetCurrentProcess(), exit_code); On 2016/11/04 11:54:17, Yang wrote: > On 2016/11/04 10:40:44, machenbach (slow) wrote: > > Do we have to wait for the process to exit? Similar to > > > https://cs.chromium.org/chromium/src/base/process/process_win.cc?q=TerminateP... > > good point. Actually, I'm pretty sure that other code is wrong and that the wait is not needed. I can see where the idea came from because the documentation is confusing. It says: "This function stops execution of all threads within the process and requests cancellation of all pending I/O. ... TerminateProcess is asynchronous; it initiates termination and returns immediately. If you need to be sure the process has terminated, call the WaitForSingleObject function with a handle to the process." The last paragraph suggests waiting. But the first paragraph says that it stops execution of all threads, which makes waiting pointless. I guess I don't know for sure, but I think that the waiting is for when you use this on another process. I'm pretty sure it's not needed for suicide. You could do a __debugbreak afterwards and I'm pretty sure it would never get hit. Harmless either way.
I looked at the CRT source code, specifically the _invoke_watson function used when a constraint violation is detected. It's last line is: TerminateProcess(GetCurrentProcess(), STATUS_INVALID_CRUNTIME_PARAMETER); No wait. This strongly suggests that waiting when committing suicide is not needed.
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2478473003/#ps60001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Description was changed from ========== [d8] cleanly force exit in d8 on windows. Using _exit on windows may cause race conditions in threads. BUG=chromium:603131 ========== to ========== [d8] cleanly force exit in d8 on windows. Using _exit on windows may cause race conditions in threads. BUG=chromium:603131 Committed: https://crrev.com/639f6f1774cd73d392284fa090ce012aebcaa363 Cr-Commit-Position: refs/heads/master@{#40789} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/639f6f1774cd73d392284fa090ce012aebcaa363 Cr-Commit-Position: refs/heads/master@{#40789} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
