Bug 48321 – [NRWT] Fix http lock on Windows platform

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Gabor Rapcsanyi]

Http lock doesn't work on Windows platform.

[Gabor Rapcsanyi]

Created attachment 71875 [details]
proposed patch

[Dirk Pranke]

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"

[Tony Chang]

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.

[Dirk Pranke]

(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.

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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 ...

[Gabor Rapcsanyi]

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.

[WebKit Commit Bot]

Comment on attachment 72003 [details]
proposed_patch_v2

Clearing flags on attachment: 72003

Committed r70672: <http://trac.webkit.org/changeset/70672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.