| Summary: | [NRWT] Fix http lock on Windows platform | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gabor Rapcsanyi <rgabor> | ||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abecsi, commit-queue, dpranke, eric, ojan, ossy, tony | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 48053 | ||||||||
| Attachments: |
|
||||||||
|
Description
Gabor Rapcsanyi
2010-10-26 06:55:22 PDT
Created attachment 71875 [details]
proposed patch
Comment on attachment 71875 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=71875&action=review LGTM with the comment fixed as per below. > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:75 > + FIXME: On Windows it's always return True.""" Nit: I'd rewrite this as "FIXME: os.kill() doesn't work on Windows for checking if a pid is alive, so always return True" Comment on attachment 71875 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=71875&action=review > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:78 > + FIXME: On Windows it's always return True.""" > + if sys.platform in ('darwin', 'linux2'): > + try: > + os.kill(current_pid, 0) In webkitpy/common/system/executive.py, there's a kill_process function that should work on windows. Can we use that instead? > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:108 > + if sys.platform in ('darwin', 'linux2'): > + guard_lock_flags |= os.O_NONBLOCK Should we add 'cygwin' to the platform tuple? It looks like cygwin python has os.O_NONBLOCK, although I'm not sure if it does anything. (In reply to comment #3) > (From update of attachment 71875 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71875&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:78 > > + FIXME: On Windows it's always return True.""" > > + if sys.platform in ('darwin', 'linux2'): > > + try: > > + os.kill(current_pid, 0) > > In webkitpy/common/system/executive.py, there's a kill_process function that should work on windows. Can we use that instead? > No. That routine actually kills the process. This routine is just probing the process to see if it's alive. Frankly, I'm not a fan of using kill -0 to do that, but there doesn't seem to be an alternative that doesn't require shelling out to ps or invoking WMI on windows. > > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:108 > > + if sys.platform in ('darwin', 'linux2'): > > + guard_lock_flags |= os.O_NONBLOCK > > Should we add 'cygwin' to the platform tuple? It looks like cygwin python has os.O_NONBLOCK, although I'm not sure if it does anything. I doubt it does anything. We started the process. So we should have a Popen object no? If so, we can just use poll() to check if a process is alive. (In reply to comment #5) > We started the process. So we should have a Popen object no? If so, we can just use poll() to check if a process is alive. Not if you're running multiple NRWTs and one of the other ones is the one holding the lock ... Created attachment 72003 [details]
proposed_patch_v2
As Dirk said I don't want to kill the process, just check it. I think that's the easiest way to do that.
Furthermore I've checked the os.O_NONBLOCK and it's no need because when it can't open the guard lock file it just throw an OSError exception, so it will be a spinlock without the os.O_NONBLOCK.
Comment on attachment 72003 [details] proposed_patch_v2 Clearing flags on attachment: 72003 Committed r70672: <http://trac.webkit.org/changeset/70672> All reviewed patches have been landed. Closing bug. |