Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444233002:
Not wait cloudtail finish forerver on Windows. (Closed)
|
Created:
4 years, 1 month ago by Yoshisato Yanagisawa Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionNot wait cloudtail finish forerver on Windows.
Let me provide better way of stopping cloudtail on Windows.
1. Use WaitForSingleObject to timeout in 10 seconds.
2. use handler to waitpid instead of pid.
Also, handler is got before sending signal.
3. use signal.CTRL_C_EVENT if possible.
BUG=656846
Committed: https://chromium.googlesource.com/chromium/tools/build/+/0d3a30427b4a190ca371310172455c0ea0c9064d
Patch Set 1 #
Total comments: 6
Patch Set 2 : add docstring, close handler, etc.. #
Total comments: 4
Patch Set 3 : revised with WaitForSingleObject instead. #
Total comments: 2
Patch Set 4 : fixed comments. #Patch Set 5 : forget to write timeout. #Patch Set 6 : added message to warn it passed 10 seconds. #
Total comments: 2
Patch Set 7 : fixed minor mistake. #Patch Set 8 : fixed function comments. #Messages
Total messages: 21 (6 generated)
yyanagisawa@chromium.org changed reviewers: + vadimsh@chromium.org
yyanagisawa@chromium.org changed reviewers: + shinyak@chromium.org, tikuta@chromium.org, ukai@chromium.org
Revised the code based on the comments by Vadim in https://codereview.chromium.org/2431223008/
https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:58: def wait_termination_win(pid): doc string? https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:63: handle = win32api.OpenProcess( no need to close the handle? https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:160: except (OSError, NotDiedError) as e: no need to catch pywintypes.error ?
https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:58: def wait_termination_win(pid): On 2016/10/25 07:30:24, ukai wrote: > doc string? Done. https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:63: handle = win32api.OpenProcess( On 2016/10/25 07:30:24, ukai wrote: > no need to close the handle? Done. https://codereview.chromium.org/2444233002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:160: except (OSError, NotDiedError) as e: On 2016/10/25 07:30:24, ukai wrote: > no need to catch pywintypes.error ? Since pywintypes.error might not be defined in Posix, let me catch all here.
https://codereview.chromium.org/2444233002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:84: os.waitpid(handle, 0) I think it would be simpler to use WaitForSingleObject with timeout instead of 'multiprocessing' (now that use have process HANDLE and access to WinAPI). https://codereview.chromium.org/2444233002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:98: return e this is weird... is it because of 'multiprocessing'?
Description was changed from ========== Not wait cloudtail finish forerver on Windows. Let me provide better way of stopping cloudtail on Windows. 1. waitpid timeout in 10 seconds. 2. use handler to waitpid instead of pid. Also, handler is got before sending signal. 3. use signal.CTRL_C_EVENT if possible. BUG=656846 ========== to ========== Not wait cloudtail finish forerver on Windows. Let me provide better way of stopping cloudtail on Windows. 1. Use WaitForSingleObject to timeout in 10 seconds. 2. use handler to waitpid instead of pid. Also, handler is got before sending signal. 3. use signal.CTRL_C_EVENT if possible. BUG=656846 ==========
https://codereview.chromium.org/2444233002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:84: os.waitpid(handle, 0) On 2016/10/25 18:35:41, Vadim Sh. wrote: > I think it would be simpler to use WaitForSingleObject with timeout instead of > 'multiprocessing' (now that use have process HANDLE and access to WinAPI). Done. https://codereview.chromium.org/2444233002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:98: return e On 2016/10/25 18:35:41, Vadim Sh. wrote: > this is weird... is it because of 'multiprocessing'? right. revised without multiprocessing.
https://codereview.chromium.org/2444233002/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:90: ret = win32event.WaitForSingleObject(handle) I think you forgot to specify timeout here.
https://codereview.chromium.org/2444233002/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:90: ret = win32event.WaitForSingleObject(handle) On 2016/10/26 01:21:16, Vadim Sh. wrote: > I think you forgot to specify timeout here. good catch.
lgtm, but it would be great if you can test it locally on Windows before committing I don't have access to my Windows machine currently :(
https://codereview.chromium.org/2444233002/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:69: instance of Exception: if raised unexpectedly. this statement can be better?
https://codereview.chromium.org/2444233002/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2444233002/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:69: instance of Exception: if raised unexpectedly. On 2016/10/26 01:34:05, tikuta wrote: > this statement can be better? Done.
lgtm
The CQ bit was checked by yyanagisawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2444233002/#ps140001 (title: "fixed function comments.")
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.
Description was changed from ========== Not wait cloudtail finish forerver on Windows. Let me provide better way of stopping cloudtail on Windows. 1. Use WaitForSingleObject to timeout in 10 seconds. 2. use handler to waitpid instead of pid. Also, handler is got before sending signal. 3. use signal.CTRL_C_EVENT if possible. BUG=656846 ========== to ========== Not wait cloudtail finish forerver on Windows. Let me provide better way of stopping cloudtail on Windows. 1. Use WaitForSingleObject to timeout in 10 seconds. 2. use handler to waitpid instead of pid. Also, handler is got before sending signal. 3. use signal.CTRL_C_EVENT if possible. BUG=656846 Committed: https://chromium.googlesource.com/chromium/tools/build/+/0d3a30427b4a190ca371... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/tools/build/+/0d3a30427b4a190ca371... |
