Skip to content

[symfony/lock] Lock always expired #31426

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
Closed
slince opened this issue May 8, 2019 · 3 comments
Closed

[symfony/lock] Lock always expired #31426

slince opened this issue May 8, 2019 · 3 comments
Labels
Bug Lock Status: Needs Review

Comments

@slince
Copy link

slince commented May 8, 2019 •
edited
Loading

Symfony version(s) affected: 3.4.27

Description

When multiple processes get a lock, the lock expired exception will always appear, no any processes can successfully acquire the lock.

How to reproduce

$store = new RedisStore($redisInstance, 20);
$store = new RetryTillSaveStore($store);
$factory = new Factory($store);
  
$lock = $factory->createLock('foo', 10); 
$lock->acquire(true);
   // do something
$lock->release();

Additional context

RedisStore $initialTtl = 20

Assuming the current time is 10:00s, the process "A" and the process "B" are ready to acquire the lock "foo", "foo" will be released at 10:20;

At 10:20s:

A successfully acquires the lock, accourding to

symfony/src/Symfony/Component/Lock/Store/RedisStore.php

Line 57 in a9bb118

elseif redis.call("SET", KEYS[1], ARGV[1], "NX", "PX", ARGV[2]) then

The expire time of the key in the redis will be modified to 10:40s ; but since the key expire time in memory is 10:20s, the A will exit because the program does not capture LockExpriedException and then A will be restart by the supervisor;

At 10:40s:

The B acquires the lock, and the redis expire time will be modified to 11:00s, but also because the expire time in memory (10:20s) timeout, B will also exit and restart;

After that, A and B get the locks in turn, and extend the expiration time by $initialTtl = 20s. but since the the expiration time in memory is not updated, no process can successfully acquire the lock.

image
@slince
Copy link
Author

slince commented May 8, 2019 •
edited
Loading

Maybe we should remove the expire time check from here.

symfony/src/Symfony/Component/Lock/Store/RedisStore.php

Line 69 in a9bb118

if ($key->isExpired()) {

@Simperfit
Copy link
Contributor

cc @jderusse

@jderusse
Copy link
Member

Thanks @slince for the reproduct case and the clear and deep investigation!

I'm working on a fix.

@jderusse jderusse mentioned this issue Jun 17, 2019
Merged
@fabpot fabpot closed this as completed Jun 17, 2019
fabpot added a commit that referenced this issue Jun 17, 2019
@fabpot
bug #32071 Fix expired lock not cleaned (jderusse)
cfc8ac0 
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 jderusse mentioned this issue Jun 17, 2019
Merged
nicolas-grekas added a commit that referenced this issue Jun 17, 2019
@nicolas-grekas
minor #32075 [Lock] Fix expired lock not cleaned in ZooKeeper (jderusse)
5af1e9e 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Lock Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @jderusse @xabbuh @Simperfit @slince @carsonbot