Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix expired lock not cleaned#32071
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| } | ||
| $this->checkNotExpired($key); |
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.
this check is duplicated inStores andLock. The goal was to be sure that the check is performed even when user call the store without using ourLock facade.
But I wonder if it worth it: this leads to duplicated code and some stores forget to implement it (ie. ZooKeeper)
fabpot commentedJun 17, 2019
Thank you@jderusse. |
This PR was merged into the 3.4 branch.Discussion----------Fix expired lock not cleaned| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#31426| License | MIT| Doc PR | NAWhen a lock is acquired BUT not as fast as expected, a LockExpiredException is thrown.Issue is, that the lock is not removed which avoid other process to acquire that lock.This PR clean state of store when a LockExpiredException is triggered.note: same bug should be fixed in 4.3 in PDO and ZookeeperCommits-------9f960f3 Fix expired lock not cleaned
fabpot commentedJun 17, 2019
@jderusse Can you create a PR on 4.3 for the new PDO and Zookeeper classes? (4.3 contains your fix now) |
nicolas-grekas commentedJun 17, 2019
The CI on 4.2 is red after this PR I think. Maybe a bad merge. Could you please send a PR on 4.2 to fix it? |
nicolas-grekas commentedJun 17, 2019
OK, fixed for 4.2, sorry for the noise. |
This PR was merged into the 4.3 branch.Discussion----------[Lock] Fix expired lock not cleaned in ZooKeeper| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | ,p| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes| Fixed tickets |#31426| License | MIT| Doc PR | NAFollowing#32071 for 4.3 branchcontext:When a lock is acquired BUT not as fast as expected, a LockExpiredException is thrown.Issue is, that the lock is not removed which avoid other process to acquire that lock.This PR clean state of store when a LockExpiredException is triggered in PDO and ZooKeepeer.Commits-------4f808ef Fix Expiring lock in PDO and ZooKeeper
| if ($this->key->isExpired()) { | ||
| try { | ||
| $this->release(); | ||
| }catch (\Exception$e) { |
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.
Would be nice to add it as previous$e.
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.
hmm.. I don't think so.. Here the exception throw byrelease is not related to theLockExpiredException we want to throw..
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.
Imagine having this caught in production and not being able to see the error.
Uh oh!
There was an error while loading.Please reload this page.
When a lock is acquired BUT not as fast as expected, a LockExpiredException is thrown.
Issue is, that the lock is not removed which avoid other process to acquire that lock.
This PR clean state of store when a LockExpiredException is triggered.
note: same bug should be fixed in 4.3 in PDO and Zookeeper