Bug 49187 – [NRWT] Make http locking similar to perl implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

I wrote why we need it:
https://bugs.webkit.org/show_bug.cgi?id=49164#c4

I think the best way to solve this problem if we use flock and locking depends on platform:
...
sequential_guard_lock = os.open(self._guard_lock_file, os.O_CREAT)
if windows:
    msvcrt.locking(sequential_guard_lock, msvcrt.LK_LOCK | msvcrt.LK_NBLCK, 32)
else:
    fcntl.flock(sequential_guard_lock, fcntl.LOCK_EX)
...

[Eric Seidel (no email)]

Does python provide a file locking API which hides these platform differences, or do we need to build our own?

[Eric Seidel (no email)]

Maybe we should autoinstall lockfile.py?
http://packages.python.org/lockfile/lockfile.html

[Eric Seidel (no email)]

A more recent link: http://pypi.python.org/pypi/lockfile/0.9.1

[Csaba Osztrogonác]

Thanks for the link, this lockfile API would be better than my previous idea.
Gábor, could you check it, please?

[Eric Seidel (no email)]

If you wish to autoinstall packages look at webkitpy.thirdparty.autoinstalled.__init__.py for examples.

[Gabor Rapcsanyi]

(In reply to comment #2)
> Maybe we should autoinstall lockfile.py?
> http://packages.python.org/lockfile/lockfile.html

This lock file is not good for us:

http://packages.python.org/lockfile/lockfile.html
"""Note The current implementation doesn’t provide for shared vs. exclusive locks. It should be possible for multiple reader processes to hold the lock at the same time."""

And we need exclusive lock.

[Gabor Rapcsanyi]

Created attachment 73378 [details]
proposed patch

I created a new FileLock class and I added unittests for it.

[Tony Chang]

Comment on attachment 73378 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73378&action=review

Just some small nits in the unittest.  The approach seems fine overall.

> WebKitTools/Scripts/webkitpy/common/system/file_lock.py:27
> +"""This class helps to lock files exclusively."""

Nit: maybe mention that this is across processes.

> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:38
> +        self._lock_path = os.path.join(tempfile.gettempdir(), "TestWebKit.lock")
> +        self._file_lock = FileLock(self._lock_path)

Can you run this in setUp() instead?

> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:48
> +        # Create the lock.
> +        self._file_lock.acquire_lock()
> +        self.assertTrue(os.path.exists(self._lock_path))

What happens if two bots that run on the same machine try to run python-tests at the same time?  Maybe we can randomize the lock name.

[Eric Seidel (no email)]

Comment on attachment 73378 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73378&action=review

>> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:38
>> +    def __init__(self, testFunc):
>> +        self._lock_path = os.path.join(tempfile.gettempdir(), "TestWebKit.lock")
>> +        self._file_lock = FileLock(self._lock_path)
> 
> Can you run this in setUp() instead?

Yes, as tony says, thsi shoudl be in setUp() since its torn down in tearDown.

> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:60
> +    def test_stuck_lock(self):
> +        open(self._lock_path, 'w')
> +        self._file_lock.acquire_lock()
> +        self._file_lock.release_lock()

What is this expected to do?  Do we always just blow away the lock?  in which case, how is it a lock?

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:100
> -        except IOError, OSError:
> +        except (IOError, OSError):

Are () the pep8 recommendation?

[Peter Gal]

(In reply to comment #9)
> > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:100
> > -        except IOError, OSError:
> > +        except (IOError, OSError):
> 
> Are () the pep8 recommendation?

If the () is missing then it would only catch an IOError and inside the block you can access the exception with the 'OSError' named variable.
example:
try:
 ....
except IOError, OSError:
    print OSError.message


If you use the () then you can catch the IOError and the OSError also, which is what we need.

[Gabor Rapcsanyi]

Created attachment 73402 [details]
proposed_patch_02

[Tony Chang]

Comment on attachment 73402 [details]
proposed_patch_02

View in context: https://bugs.webkit.org/attachment.cgi?id=73402&action=review

> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:58
> +        open(self._lock_path, 'w')

Nit: There should be a .close() here to explicitly release the file handle.

To answer Eric's question: This is testing a stale lock file (that no one has a flock on) and the call to open is just to create the file (i.e., unix touch).

[Gabor Rapcsanyi]

(In reply to comment #8)
> (From update of attachment 73378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73378&action=review
> 
> Just some small nits in the unittest.  The approach seems fine overall.
> 
> > WebKitTools/Scripts/webkitpy/common/system/file_lock.py:27
> > +"""This class helps to lock files exclusively."""
> 
> Nit: maybe mention that this is across processes.
> 

corrected

> > WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:38
> > +        self._lock_path = os.path.join(tempfile.gettempdir(), "TestWebKit.lock")
> > +        self._file_lock = FileLock(self._lock_path)
> 
> Can you run this in setUp() instead?
> 

corrected

> > WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:48
> > +        # Create the lock.
> > +        self._file_lock.acquire_lock()
> > +        self.assertTrue(os.path.exists(self._lock_path))
> 
> What happens if two bots that run on the same machine try to run python-tests at the same time?  Maybe we can randomize the lock name.

Yes it can cause troubles, but I think adding process pid is better than a random lock name.

[Eric Seidel (no email)]

Comment on attachment 73402 [details]
proposed_patch_02

View in context: https://bugs.webkit.org/attachment.cgi?id=73402&action=review

> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:38
> +        self._lock_name = "TestWebKit" + str(os.getpid()) + ".lock"
> +        self._lock_path = os.path.join(tempfile.gettempdir(), self._lock_name)

re-reading this, why does this name ned to be different if th path is?

>> WebKitTools/Scripts/webkitpy/common/system/file_lock_unittest.py:58
>> +        open(self._lock_path, 'w')
> 
> Nit: There should be a .close() here to explicitly release the file handle.
> 
> To answer Eric's question: This is testing a stale lock file (that no one has a flock on) and the call to open is just to create the file (i.e., unix touch).

YOu could also use with (from __future__ import with_statement) to have it auto-close the file.

[Csaba Osztrogonác]

Landed in http://trac.webkit.org/changeset/71672 
And rolled out: http://trac.webkit.org/changeset/71679

Gábor please add a comment about the problem tomorrow. 
It is too late for me to do it. :(

[Gabor Rapcsanyi]

(In reply to comment #15)
> Landed in http://trac.webkit.org/changeset/71672 
> And rolled out: http://trac.webkit.org/changeset/71679
> 
> Gábor please add a comment about the problem tomorrow. 
> It is too late for me to do it. :(

As I saw when we try to unlink the exclusive lock it raises this exception:
WindowsError(32, 'The process cannot access the file because it is being used by another process')

The other problem is that it always create a new http lock when this happens, and this makes many files in the temp directory while it times out. Sorry for that mistake I'm working on the solution.

[Gabor Rapcsanyi]

Created attachment 73901 [details]
proposed_patch_v3

I fixed the Windows problem.

[Gabor Rapcsanyi]

I reorganized my previous patch.
I added a remove_lock() method to the FileLock class to unlock the locked file. On Mac and Linux it's worked without this unlock but on Windows we need this to prevent that the unlinking raises an exception.

I also put the while loop into the acquire_lock() method, and when it times out it will return false. I think it is easier to handle it. I set the default wait time to 20 seconds, it has to be enough for the file creation.
If something goes wrong it wont do any locking, just run the http tests without it.

[Tony Chang]

Comment on attachment 73901 [details]
proposed_patch_v3

View in context: https://bugs.webkit.org/attachment.cgi?id=73901&action=review

> WebKitTools/Scripts/webkitpy/common/system/file_lock.py:36
> +    def __init__(self, lock_file_path, max_wait_time=20):

Nit: I prefer to have the units included in time variables.  E.g., max_wait_time_sec

> WebKitTools/Scripts/webkitpy/common/system/file_lock.py:65
> +            except IOError:
> +                if time.time() - start_time > self._max_wait_time:

Maybe we should log the error here for debugging purposes?

> WebKitTools/Scripts/webkitpy/common/system/file_lock.py:78
> +        except (IOError, OSError):
> +            pass

Should we log the error here too?

[Csaba Osztrogonác]

(In reply to comment #19)
> (From update of attachment 73901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73901&action=review
> 
> > WebKitTools/Scripts/webkitpy/common/system/file_lock.py:36
> > +    def __init__(self, lock_file_path, max_wait_time=20):
> 
> Nit: I prefer to have the units included in time variables.  E.g., max_wait_time_sec
> 
> > WebKitTools/Scripts/webkitpy/common/system/file_lock.py:65
> > +            except IOError:
> > +                if time.time() - start_time > self._max_wait_time:
> 
> Maybe we should log the error here for debugging purposes?
> 
> > WebKitTools/Scripts/webkitpy/common/system/file_lock.py:78
> > +        except (IOError, OSError):
> > +            pass
> 
> Should we log the error here too?

Gábor fixed and I landed it: http://trac.webkit.org/changeset/72194