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.

Sign up for GitHub

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

Jump to bottom

refactor: Use native Lock timeout instead of reimplementing #676

Merged
merged 1 commit into from
Oct 30, 2022
Merged

refactor: Use native Lock timeout instead of reimplementing #676

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 7 additions & 17 deletions kazoo/recipe/lock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def __init__(self, client, path, identifier=None, extra_lock_patterns=()):
self._retry = KazooRetry(
max_tries=None, sleep_func=client.handler.sleep_func
)
self._lock = client.handler.lock_object()
self._acquire_method_lock = client.handler.lock_object()

def _ensure_path(self):
self.client.ensure_path(self.path)
Expand Down Expand Up @@ -174,27 +174,17 @@ def acquire(self, blocking=True, timeout=None, ephemeral=True):
The ephemeral option.
"""

def _acquire_lock():
got_it = self._lock.acquire(False)
if not got_it:
raise ForceRetryError()
return True

retry = self._retry.copy()
retry.deadline = timeout

# Ensure we are locked so that we avoid multiple threads in
# this acquistion routine at the same time...
locked = self._lock.acquire(False)
if not locked and not blocking:
method_locked = self._acquire_method_lock.acquire(
blocking=blocking, timeout=timeout if timeout is not None else -1
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan that python's threading chose to require timeout to be a number. Thankfully, it seems the other handler implementation's underlying Lock replacement also follows the python API, so this should be safe.

I could just replace the default parameter to -1 instead. Don't have a strong opinion either way

Copy link
Member

Choose a reason for hiding this comment

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

IMO the way you implemented it is better: it does not change the method signature, so no chance to break someone's current code.

)
if not method_locked:
return False
if not locked:
# Lock acquire doesn't take a timeout, so simulate it...
# XXX: This is not true in Py3 >= 3.2
try:
locked = retry(_acquire_lock)
except RetryFailedError:
return False

already_acquired = self.is_acquired
try:
gotten = False
Expand All @@ -220,7 +210,7 @@ def _acquire_lock():
self._best_effort_cleanup()
return gotten
finally:
self._lock.release()
self._acquire_method_lock.release()

def _watch_session(self, state):
self.wake_event.set()
Expand Down