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

Fix expired lock not cleaned #32071

Merged
merged 1 commit into from Jun 17, 2019
Merged

Fix expired lock not cleaned #32071

merged 1 commit into from Jun 17, 2019

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Jun 17, 2019

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

@@ -78,6 +78,8 @@ public function save(Key $key)
}
}

$this->checkNotExpired($key);
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

@fabpot
Copy link
Member

fabpot commented Jun 17, 2019

Thank you @jderusse.

@fabpot fabpot merged commit 9f960f3 into symfony:3.4 Jun 17, 2019
fabpot added a commit that referenced this pull request Jun 17, 2019
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
@fabpot
Copy link
Member

fabpot commented Jun 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
Copy link
Member

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?
See https://travis-ci.org/symfony/symfony/jobs/546872044

@nicolas-grekas
Copy link
Member

OK, fixed for 4.2, sorry for the noise.
For 4.3, I don't know how to apply the change so I'll let you do it. Thanks

nicolas-grekas added a commit that referenced this pull request Jun 17, 2019
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
This was referenced Jun 26, 2019
@jderusse jderusse deleted the fix-expired-lock branch August 2, 2019 12:15
@@ -83,6 +83,11 @@ public function acquire($blocking = false)
}

if ($this->key->isExpired()) {
try {
$this->release();
} catch (\Exception $e) {
Copy link
Contributor

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.

Copy link
Member Author

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 by release is not related to the LockExpiredException we want to throw..

Copy link
Contributor

@umpirsky umpirsky Nov 12, 2020

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.

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

Successfully merging this pull request may close these issues.

None yet

5 participants