Fix expired lock not cleaned #32071
Fix expired lock not cleaned #32071
Conversation
2c6fca7 to
9f960f3
Compare
| @@ -78,6 +78,8 @@ public function save(Key $key) | |||
| } | |||
| } | |||
|
|
|||
| $this->checkNotExpired($key); | |||
There was a problem hiding this comment.
this check is duplicated in Stores and Lock. The goal was to be sure that the check is performed even when user call the store without using our Lock facade.
But I wonder if it worth it: this leads to duplicated code and some stores forget to implement it (ie. ZooKeeper)
|
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 | NA 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 Commits ------- 9f960f3 Fix expired lock not cleaned
|
@jderusse Can you create a PR on 4.3 for the new PDO and Zookeeper classes? (4.3 contains your fix now) |
|
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? |
|
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 <!-- see https://symfony.com/bc --> | Deprecations? | no | Tests pass? | yes | Fixed tickets | #31426 | License | MIT | Doc PR | NA Following #32071 for 4.3 branch context: 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
| @@ -83,6 +83,11 @@ public function acquire($blocking = false) | |||
| } | |||
|
|
|||
| if ($this->key->isExpired()) { | |||
| try { | |||
| $this->release(); | |||
| } catch (\Exception $e) { | |||
There was a problem hiding this comment.
Would be nice to add it as previous $e.
There was a problem hiding this comment.
hmm.. I don't think so.. Here the exception throw by release is not related to the LockExpiredException we want to throw..
There was a problem hiding this comment.
Imagine having this caught in production and not being able to see the error.
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