Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569643002:
Add logging in Executive.check_running_pid. (Closed)
|
Created:
4 years ago by qyearsley Modified:
4 years ago Reviewers:
Dirk Pranke CC:
blink-reviews, chromium-reviews, Dirk Pranke, jeffcarp, tkent Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd logging in Executive.check_running_pid.
The purpose of this CL is to add logging in an attempt to confirm
whether check_running_pid is called before a hang occurs when
shutting down wptserve.
BUG=669802
Committed: https://crrev.com/4d6a4b9b7ca72f5ed0d7a25aa62d8dbb22ce1a63
Cr-Commit-Position: refs/heads/master@{#438921}
Patch Set 1 #Patch Set 2 : Remove added logging line in win32-specific method #Messages
Total messages: 18 (11 generated)
Description was changed from ========== Add logging in Executive.check_running_pid. BUG=669802 ========== to ========== Add logging in Executive.check_running_pid. Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. So, this may not be the best way to check whether a process is running in Windows. http://www.madebuild.org/blog/?p=30 and http://stackoverflow.com/questions/17620833/check-if-pid-exists-on-windows-wi... suggest checking by trying to call OpenProcess with a given pid and then checking the result of that. Another possible alternative would be to use a third-party library like psutil, which is available on the builders (since it's in infra/ENV/lib/python2.7/site-packages/) but may not be available on some developers' workstations. BUG=669802 ==========
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org
Description was changed from ========== Add logging in Executive.check_running_pid. Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. So, this may not be the best way to check whether a process is running in Windows. http://www.madebuild.org/blog/?p=30 and http://stackoverflow.com/questions/17620833/check-if-pid-exists-on-windows-wi... suggest checking by trying to call OpenProcess with a given pid and then checking the result of that. Another possible alternative would be to use a third-party library like psutil, which is available on the builders (since it's in infra/ENV/lib/python2.7/site-packages/) but may not be available on some developers' workstations. BUG=669802 ========== to ========== Add logging in Executive.check_running_pid. Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. So, this may not be the best way to check whether a process is running in Windows. http://www.madebuild.org/blog/?p=30 and http://stackoverflow.com/questions/17620833/check-if-pid-exists-on-windows-wi... suggest checking by trying to call OpenProcess with a given pid and then checking the result of that. Another possible alternative would be to use a third-party library like psutil, which is available on the builders (since it's in infra/ENV/lib/python2.7/site-packages/), but may not be available on some developers' workstations. BUG=669802 ==========
Description was changed from ========== Add logging in Executive.check_running_pid. Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. So, this may not be the best way to check whether a process is running in Windows. http://www.madebuild.org/blog/?p=30 and http://stackoverflow.com/questions/17620833/check-if-pid-exists-on-windows-wi... suggest checking by trying to call OpenProcess with a given pid and then checking the result of that. Another possible alternative would be to use a third-party library like psutil, which is available on the builders (since it's in infra/ENV/lib/python2.7/site-packages/), but may not be available on some developers' workstations. BUG=669802 ========== to ========== Add logging in Executive.check_running_pid. Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. The purpose of this CL is to add logging in an attempt to confirm whether this is happening. BUG=669802 ==========
I just remembered that we were seeing this happening on Mac and Linux but not Windows -- so the logging in check_running_pid could help to confirm whether check_running_pid is called, but the logging in _win32_check_running_pid might just add a lot of verbose logging without really helping.
Description was changed from ========== Add logging in Executive.check_running_pid. Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. The purpose of this CL is to add logging in an attempt to confirm whether this is happening. BUG=669802 ========== to ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. -- Original description -- Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. BUG=669802 ==========
yup, i'd get rid of the windows line. lgtm.
Description was changed from ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. -- Original description -- Looking at this Executive._win32_check_running_pid, which was added in https://trac.webkit.org/changeset/71338, I think that it's possible that Windows may hang in the infinite loop there if kernel32.Process32Next never returns a Falsey value. BUG=669802 ========== to ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. BUG=669802 ==========
On 2016/12/15 at 02:57:56, dpranke wrote: > yup, i'd get rid of the windows line. > > lgtm. Done. Now, if we see another hang when starting wptserve and it looks the same as before but with a log from check_running_pid, then that would suggest that it's hanging somewhere in this method.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2569643002/#ps20001 (title: "Remove added logging line in win32-specific method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481830764285340,
"parent_rev": "562331a081e9295d3f16ea35d57b767981fa7f9e", "commit_rev":
"912a60821002c8851f639e7a103ff12d6aaec3ad"}
Message was sent while issue was closed.
Description was changed from ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. BUG=669802 ========== to ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. BUG=669802 Review-Url: https://codereview.chromium.org/2569643002 Committed: https://chromium.googlesource.com/chromium/src/+/912a60821002c8851f639e7a103f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/912a60821002c8851f639e7a103f...
Message was sent while issue was closed.
Description was changed from ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. BUG=669802 Review-Url: https://codereview.chromium.org/2569643002 Committed: https://chromium.googlesource.com/chromium/src/+/912a60821002c8851f639e7a103f... ========== to ========== Add logging in Executive.check_running_pid. The purpose of this CL is to add logging in an attempt to confirm whether check_running_pid is called before a hang occurs when shutting down wptserve. BUG=669802 Committed: https://crrev.com/4d6a4b9b7ca72f5ed0d7a25aa62d8dbb22ce1a63 Cr-Commit-Position: refs/heads/master@{#438921} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4d6a4b9b7ca72f5ed0d7a25aa62d8dbb22ce1a63 Cr-Commit-Position: refs/heads/master@{#438921} |
