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

[RFC][lock] Introduce Shared Lock (or Read/Write Lock) #37752

Merged
merged 1 commit into from Sep 12, 2020

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Aug 6, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR TODO

This PR adds a new method "acquireRead" to the Lock class in order to solve the single writer multiple readers problem.

usage:

$aliceLock = $factory->createLock('invoice');
$bobLock = $factory->createLock('invoice');
$oscarLock = $factory->createLock('invoice');

$aliceLock->acquireRead(); // true
$bobLock->acquireRead(); // true
$oscarLock->acquire(); // false

next steps

add more stores

  • pdo
  • memcached
  • zookeeper
  • mongodb

Priority policies

Priority policy (read-preffering or write preffering) is not covered by this PR.

Promote/Demote

Converting a Read lock to Write Lock (promote) or Write lock to Read lock (demote) is covered by calling acquireRead / acquired method.

// demote
$lock->acquire();
...
$lock->acquireRead(); 

// promote
$lock->acquireRead();
...
$lock->acquire(); 

@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) Lock Feature labels Aug 6, 2020
@jderusse jderusse requested a review from lyrixx August 6, 2020 08:46
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

Do stores deal properly with an upgrade scenario, where the store might contain existing locks created using the old format for the state ?

@jderusse
Copy link
Member Author

jderusse commented Aug 6, 2020

Do stores deal properly with an upgrade scenario, where the store might contain existing locks created using the old format for the state ?

Good point. For sure, they have to.
But I don't know how to add test to assert it.

@stof
Copy link
Member

stof commented Aug 6, 2020

Good point. For sure, they have to.

I know they have to. The question is whether they actually do.

To test this, we would need to write some lock in the old format in Redis (or whatever storage) and then try to acquire the same lock with the new code and ensure it does not break but properly detects a conflict.

For a local test, what you can do is creating 2 projects: one using symfony/lock 5.1 to acquire the lock and hold it, and another one using your new code and trying to acquire a (conflicting) lock (should be tested with both acquire() and acquireRead(). This might be easier than filling the storage by hand.

@jderusse
Copy link
Member Author

jderusse commented Aug 6, 2020

I tested compatibility locally with 2 clients (with different versions) at the same time and it worked in both way.
(tested Flock and Redis)

I also added a integration test that reuse the old code to assert it for Redis.

@jderusse jderusse mentioned this pull request Aug 13, 2020
3 tasks
@fabpot
Copy link
Member

fabpot commented Aug 19, 2020

@jderusse Just for my information, the next step here is to implement the feature in other stores?

@jderusse
Copy link
Member Author

@jderusse Just for my information, the next step here is to implement the feature in other stores?

That was my first idea. But it's more complicated than I thought, and PR might be harder to review. I suggest:

  • decide if we like the idea/rfc and merge this PR first (with 2 stores implemented)
  • create 1 PR dedicated per store latter (5.2 if possible)

@fabpot
Copy link
Member

fabpot commented Aug 19, 2020

Having this feature available for some stores only is fine by me.

@fabpot fabpot added this to the next milestone Aug 23, 2020
@fabpot
Copy link
Member

fabpot commented Sep 2, 2020

@jderusse Having additionnal stores in other PRs is a good idea. Can you finish this one without other stores?

local uniqueToken = ARGV[1]
local ttl = tonumber(ARGV[2])

local now = redis.call("TIME")
Copy link
Member Author

Choose a reason for hiding this comment

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

TIME is not deterministic before Redis 5.0 (see #35780 (comment))

But using php's time (multiple source/server) is not reliable.

What's the best solution @lyrixx ?

Copy link
Member

Choose a reason for hiding this comment

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

In semaphore I used the server's time. I think it's more reliable than hopping user are running redis > 5.X

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I did not realized that the script of REDIS < 5 was broken because of that.
Thanks for the confirmation, I've refactored this part.

@jderusse jderusse force-pushed the lock-read branch 4 times, most recently from 151a090 to f0feb6d Compare September 2, 2020 15:41
@jderusse
Copy link
Member Author

jderusse commented Sep 2, 2020

PR is ready for review, with the current 2 stores (flock, redis)

@jderusse jderusse force-pushed the lock-read branch 2 times, most recently from 7679a1e to c1a8862 Compare September 2, 2020 22:59
local uniqueToken = ARGV[2]
local ttl = tonumber(ARGV[3])

-- asserts the KEY is compatible with current version (old Symfony <5.2 BC)
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 provide backward compatibility:

  • when an old version of symfony put a lock, the new version won't be able to put a lock.

But the opposite is not (always) true:

  • with predis: when a new version of symfony put a lock, the old version will fail with an ServerException
  • with phpredis it's ok: when an new version of symfony put a lock, the old version won't be able to put a lock.

Both situation are safe (user won't be able to acquire the lock), but the complexity of the code to throw a proper "LockConfictException" instead of "predis\ServerException" don't worth it IMHO

@jderusse jderusse force-pushed the lock-read branch 2 times, most recently from a50509e to 4789f54 Compare September 2, 2020 23:16
fabpot
fabpot approved these changes Sep 12, 2020
@fabpot
Copy link
Member

fabpot commented Sep 12, 2020

Thank you @jderusse.

@fabpot fabpot merged commit d7d479b into symfony:master Sep 12, 2020
1 of 3 checks passed
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Sep 22, 2020
…usse)

This PR was merged into the master branch.

Discussion
----------

[Lock] Add documentation about Shared Locks

Fixes symfony#14225 implemented in symfony/symfony#37752

Commits
-------

dfd97c5 Add documentation about read/write locks
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
@jderusse jderusse deleted the lock-read branch October 15, 2020 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Lock RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants