Does not successfully lock on Windows

[csm10495]

Hi,

On Windows 10.0.19041.687 Pro x64. Python 3.7.0: x64.

Here is a test script:

import tempfile
import pathlib
import threading
from concurrent.futures import ProcessPoolExecutor, ThreadPoolExecutor
from filelock import FileLock
import time

TEST_FILE = pathlib.Path(tempfile.gettempdir()) / 'test_file.txt'
TEST_LOCK_FILE =  pathlib.Path(tempfile.gettempdir()) / 'test_file.txt.lock'
LOCK = FileLock(TEST_LOCK_FILE)

def test():
    with LOCK:
        assert TEST_FILE.read_text() == 'hi'
        TEST_FILE.write_text('')
        assert TEST_FILE.read_text() == ''
        TEST_FILE.write_text('hi')
        assert TEST_FILE.read_text() == 'hi'
        return True

if __name__ == '__main__':
    print(f"Test file: {TEST_FILE}")
    print(f"Test lock file: {TEST_LOCK_FILE}")
    TEST_FILE.write_text('hi')

    results = []

    # works with ProcessPoolExecutor but not with ThreadPoolExecutor
    # It also is more likely to work if we sleep after calling submit()
    with ThreadPoolExecutor(max_workers=16) as pool:
        for i in range(100):
            if i % 10 == 0:
                print (f"Loop: {i+1}")
            results.append(pool.submit(test))

        for idx, result in enumerate(results):
            print (f"Checking result: {idx + 1}")
            assert result.result() == True

Example run:

PS C:\Users\csm10495\Desktop> python .\file_lock_test.py
Test file: C:\Users\csm10495\AppData\Local\Temp\test_file.txt
Test lock file: C:\Users\csm10495\AppData\Local\Temp\test_file.txt.lock
Loop: 1
Loop: 11
Loop: 21
Loop: 31
Loop: 41
Loop: 51
Loop: 61
Loop: 71
Loop: 81
Loop: 91
Checking result: 1
Traceback (most recent call last):
  File ".\file_lock_test.py", line 38, in <module>
    assert result.result() == True
  File "C:\Python37\lib\concurrent\futures\_base.py", line 425, in result
    return self.__get_result()
  File "C:\Python37\lib\concurrent\futures\_base.py", line 384, in __get_result
    raise self._exception
  File "C:\Python37\lib\concurrent\futures\thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File ".\file_lock_test.py", line 18, in test
    assert TEST_FILE.read_text() == 'hi'
AssertionError

Using a ThreadPoolExecutor seems to lead to assertion errors making me think the lock file wasn't atomically created. If I sleep a bit (like .01), after doing submit() it seems to work, but sort of defeats the purpose of the test.

[cbehopkins]

I've just been looking at this, the problem seems to be:

            try:
                msvcrt.locking(fd, msvcrt.LK_NBLCK, 1)
            except (IOError, OSError):
                os.close(fd)

Consulting: https://docs.python.org/3/library/msvcrt.html "Locks the specified bytes. If the bytes cannot be locked, OSError is raised." i.e. if we can't obtain the lock, then ignore the error telling us that we cannot obtain the lock.

Looks to me that _aquire needs the ability to report that it failed and a new attempt to be made.

[inverse]

@cbehopkins if we could create a test case it should be caught on #85 which adds Windows as a target

[cbehopkins]

Turns out it was PEBKAC, but the pattern to using it with pytest - what directed me here - if you are trying to do other file operations if you are also trying to clean up afterwards, is non-trivial and masquerades as FileLock not working. Literally got it reliable across multiple machines yesterday, so I should find a way to publish the recipe.

My mistake in the understanding above comes from not realising the design was such that the lock is per thread, not absolute. i.e.:

locker = FileLock("dbg_locking.lock")
assert not locker.is_locked
locker.acquire(timeout=1)
assert locker.is_locked
locker.acquire(timeout=0.1)

does not raise an exception, as the second lock acquire gets the lock just fine(as it knows this thread has the lock). This clouded my understanding as I have not experienced a lock behaving like this previously.

[gaborbernat]

Hello, if you make a PR for this (with tests) we would be happy to review it, thanks!

[gaborbernat]

We should probably document better that our platform locks are re-entrant.

[TheMatt2]

This issue seems to also apply to MacOS 12.6.3 as well (just to confirm it is not a Windows specific issue). Python 3.11.2

Test file: /var/.../test_file.txt
Test lock file: /var/.../test_file.txt.lock
Loop: 1
Loop: 11
Loop: 21
Loop: 31
Loop: 41
Loop: 51
Loop: 61
Loop: 71
Loop: 81
Loop: 91
Checking result: 1
Checking result: 2
Traceback (most recent call last):
  File ".../lock_test.py", line 38, in <module>
    assert result.result() == True
           ^^^^^^^^^^^^^^^
  File ".../concurrent/futures/_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File ".../concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File ".../concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../lock_test.py", line 14, in test
    assert TEST_FILE.read_text() == 'hi'
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

Perhaps clearer documentation that filelock shares the lock between threads? This is different than the lock just being re-entrant. Perhaps provide a filelock that does not share between threads?

[Arcitec]

Perhaps provide a filelock that does not share between threads?

The threads are fake. Python isn't freely multi-threaded. There's just 1 process in Python, and it only runs 1 thread at a time via the "Global Interpreter Lock (GIL)", which divides up the work using fakery.

The locks are at the file-level and are registered via OS-kernel features. The OS cannot differentiate between the different fake Python threads. It's all just "that Python process" to the OS, so no it's not possible to do this differently.

And those OS-kernel locking features are used to ensure that there aren't race conditions and other issues that homebrew locking solutions always have.

[csm10495]

I've opened a PR to resolve this: https://github.com/tox-dev/py-filelock/pull/219.. It was something that hit me all of a sudden and makes complete sense. By forcing the WindowsLock to be thread-local, it avoids any weird issues that can crop up with multiple python threads and the os thinking that its the same process so unlock, etc.

[csm10495]

If we wanted to 'fix' this everywhere maybe we should just make BaseFileLock inherit from threading.local.

Edit: I've updated the PR to do this since folks have hit this in posix-land too.