-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(lock): Fix LockError message when releasing a lock. #3534
fix(lock): Fix LockError message when releasing a lock. #3534
Conversation
Hi @shenxiangzhuang, thank you for your contribution! Your change will be reviewed soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original code, we have the following:
def release(self) -> None:
"""
Releases the already acquired lock
"""
expected_token = self.local.token
if expected_token is None:
raise LockError("Cannot release an unlocked lock", lock_name=self.name)
self.local.token = None
self.do_release(expected_token)
This code can raise the LockError in several cases(Refering in some of the cases to the example in the reported issue):
- When the token object is not initialized or is already reset to None - it can happen if in thread 1 we have already called release and we call it again - in this case, the error handling is fine - the lock has really been already unlocked.
- We are in another thread that hasn't acquired a lock - which sounds like the message and meaning of LockNotOwnedError
- We might get in a similar to the first situation using the locks with context manager and trying to call the release function additionally.
If we are in Thread1 and we call release after the lock has expired, we get to self.do_release(expected_token)
which raises the LockNotOwnedError with message "Cannot release a lock that's no longer owned" - which sound correct.
Overall I think that we can just change the Exception to LockNotOwnedError(won't be breaking change, because it is a subtype of the LockError) with a proper error message, something like "Cannot release a lock that's not owned or is already unlocked."
I think it does a more proper way to ease the problem in #3535 and I have changed the code, please review it again! |
@shenxiangzhuang Code linters still failing, apart of it it's okay |
@shenxiangzhuang, thank you very much for your efforts. Can you please apply the change to the async lock as well? The code there is identical. |
@petyaslavova , ok, I have changed the async implementation. Please review it again. cc @vladvildanov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fix #3535
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
We can get expect result for the test code in #3535 with this implementation: