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

Support reentrant locking on lock file path via optional singleton instance #283

Merged
merged 12 commits into from
Oct 27, 2023
Merged

Support reentrant locking on lock file path via optional singleton instance #283

merged 12 commits into from
Oct 27, 2023

Conversation

nefrob
Copy link
Contributor

@nefrob nefrob commented Oct 25, 2023

Description

Re: #282.

  • When _singleton_per_lock_file is set instantiating a new lock with the same lock path will return the existing instance of the lock. This means you can reacquire the lock without passing the lock object around
  • Otherwise a new lock object is returned every time (default behavior for backwards compatibility)

Based on the flock docs (and likely similar for windows/soft cases)

If a process uses open(2) (or similar) to obtain more than one
file descriptor for the same file, these file descriptors are
treated independently by flock(). An attempt to lock the file
using one of these file descriptors may be denied by a lock that
the calling process has already placed via another file
descriptor.

(https://man7.org/linux/man-pages/man2/flock.2.html)

a Python solution seems most general and easy here. If anyone has time to dig into different OS file locking mechanisms and see if there is a native solution that would likely be preferable.

Action Items

  • docs updates?
  • concurrency? If the use case fro non-thread-local locks is to create the lock on the main thread, share it with threads, then cleanup the lock when they complete then this should be safe as the threads will only ever read the instance from the class instances dictionary. If supporting hand-off of the lock to thread processes is within the thread-local options then we should acquire a global lock around the instances dictionary for safety (at the cost of speed)

src/filelock/_api.py Outdated Show resolved Hide resolved
@nefrob nefrob changed the title ✨ Support reentrant locking without passing the lock object around ✨ Support reentrant locking on lock file path Oct 25, 2023
src/filelock/_api.py Outdated Show resolved Hide resolved
@nefrob nefrob marked this pull request as ready for review October 26, 2023 22:25
src/filelock/_api.py Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@nefrob nefrob changed the title ✨ Support reentrant locking on lock file path Support reentrant locking on lock file path via optional singleton instance Oct 27, 2023
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
src/filelock/_api.py Outdated Show resolved Hide resolved
@nefrob
Copy link
Contributor Author

nefrob commented Oct 27, 2023

Downloading pypy locally to investigate test failures. Not sure what the strategy for the type failures is (ex. switch from Sequence to list, drop the type entirely and noqa or resolve via getting type stubs for mypy?). I misunderstoof mypy errors, it just wants type hints for the data inside the container objects.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Don't disable ruff checks, address them.

src/filelock/_api.py Outdated Show resolved Hide resolved
src/filelock/_api.py Outdated Show resolved Hide resolved
src/filelock/_api.py Outdated Show resolved Hide resolved
nefrob and others added 2 commits October 27, 2023 10:52
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 3e3455e into tox-dev:main Oct 27, 2023
31 checks passed
@nefrob
Copy link
Contributor Author

nefrob commented Oct 27, 2023

Thanks so much for the help!

@YouJiacheng
Copy link

YouJiacheng commented Mar 24, 2024

I think there might be a bug: all derived class of BaseFileLock share the same _instances:

from filelock import UnixFileLock, BaseFileLock

assert UnixFileLock._instances is BaseFileLock._instances

This can be resolved by meta class or simpler __init_subclass__ (python>=3.6)

@YouJiacheng
Copy link

BTW, maybe we should check if args like mode and timeout is the same as the singleton?

@nefrob
Copy link
Contributor Author

nefrob commented Mar 25, 2024

@YouJiacheng good callout. I think I avoided a metaclass in the first place because BaseFileLock inherits from ABC which sets its metaclass already. I can take a look at this later today.

Checking mode and timeout don't seem particularly relevant if we determine two singleton locks are the sam object (so will have the same values), unless you meant something else there.

@YouJiacheng
Copy link

Okay, __init_subclass__ can be used instead of metaclass.

By checking mode and timeout, I mean: if mode and timeout are different, but lock_file are the same, there should be some warning, since the second mode and timeout will be ignored.

@nefrob
Copy link
Contributor Author

nefrob commented Mar 25, 2024

@YouJiacheng here is the fix using __init_subclass__: #318.

As for the other question, deciding on how to handle that seems like something for @gaborbernat to decide on. We could:

  1. raise an error
  2. update the existing singleton lock
  3. ignore doing anything (current behavior)

@gaborbernat
Copy link
Member

  1. raise an error

Would be my vote.

@nefrob
Copy link
Contributor Author

nefrob commented Mar 26, 2024

Fix for incompatible singleton args: #320.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants