Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Windows support to file locking #1157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions pex/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import atexit
import contextlib
import errno
import fcntl
import os
import re
import shutil
Expand All @@ -21,8 +20,14 @@
from datetime import datetime
from uuid import uuid4

from pex.compatibility import WINDOWS
from pex.typing import TYPE_CHECKING

if WINDOWS:
import msvcrt
else:
import fcntl

if TYPE_CHECKING:
from typing import Any, DefaultDict, Iterable, Iterator, NoReturn, Optional, Set, Sized

Expand Down Expand Up @@ -393,7 +398,10 @@ def unlock():
if lock_fd is None:
return
try:
fcntl.lockf(lock_fd, fcntl.LOCK_UN)
if WINDOWS:
msvcrt.locking(lock_fd, msvcrt.LK_UNLCK, 1)
else:
fcntl.lockf(lock_fd, fcntl.LOCK_UN)
finally:
os.close(lock_fd)

Expand All @@ -410,7 +418,21 @@ def unlock():
# N.B.: Since lockf operates on an open file descriptor and these are guaranteed to be
# closed by the operating system when the owning process exits, this lock is immune to
# staleness.
fcntl.lockf(lock_fd, fcntl.LOCK_EX) # A blocking write lock.
if WINDOWS:
while True:
# Force the non-blocking lock to be blocking. LK_LOCK is msvcrt's implementation of
Copy link
Member

@jsirois jsirois Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: presumably then using LK_LOCK is a bit better here. You still need to loop, but presumably its slightly more efficient to let Windows do the 1st 10 tries before sleeping here and looping than to do the 1st 10 tries here manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, it also raises a clearer error (OSError: [Errno 36] Resource deadlock avoided) when the locking operation times out after 10 tries, so we can use that directly rather than an OSError catchall. Fixed in edc8435

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent - that's much better.

# a blocking lock, but it only tries 10 times, once per second before rasing an
# OSError.
try:
msvcrt.locking(lock_fd, msvcrt.LK_LOCK, 1)
break
except OSError as ex:
# Deadlock error is raised after failing to lock the file
if ex.errno != errno.EDEADLOCK:
raise
safe_sleep(1)
else:
fcntl.lockf(lock_fd, fcntl.LOCK_EX) # A blocking write lock.
if atomic_dir.is_finalized:
# We lost the double-checked locking race and our work was done for us by the race
# winner so exit early.
Expand Down