-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[Lock] Re-add the Lock component in 3.4 #7866
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
Conversation
components/lock.rst
Outdated
The Lock Component creates and manages `locks`_, a mechanism to provide | ||
exclusive access to a shared resource. | ||
|
||
.. versionadded:: 3.3 |
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 should be changed to 3.4
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.
good catch @apfelbox
Component re-added (symfony/symfony#22597) I'll create a separate PR to fix issue related to issues |
Can we move forward with the Lock documentation ? I wish to improve the documentation to fix symfony/symfony#22452 |
Ok for me. @xabbuh? |
This reverts commit e14709d.
$lock = $factory->createLock('pdf-invoice-generation'); | ||
|
||
if ($lock->acquire()) { | ||
// The resource "pdf-invoice-generation" is locked. |
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.
Since a lock is an unmanaged resource, there is an exeception safety issue right here. Despite this is not a bug in the lock component itself, you should promote exception safety in examples IMO.
It can be fixed with a simple try / finally
$lock = $factory->createLock('pdf-invoice-generation');
if ($lock->acquire()) {
try {
// ...
} finally {
$lock->release();
}
}
You can even add an helper method to avoid this boilerplate
$lock = $factory->createLock('pdf-invoice-generation');
$lock->with(function() {
// ...
});
With the following implementation
public function with($callback)
{
if (!$this->acquire()) {
return false;
}
try {
$callback();
} finally {
$this->release();
}
return true;
}
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.
You're right, this chapter was covered in Blocking Locks
but it's better to promote it everywhere.
Usage | ||
----- | ||
|
||
Locks are used to guarantee exclusive access to some shared resource. In |
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.
Based on my experience in my company, we've spent lot of time and headaches using distributed locks for use cases where more reliable tools are best suited.
I mean:
- for high concurrency use cases, atomic write operations are better when available.
- for low concurrency use cases, OCC is better when available.
For example, to avoid concurrent business transactions on the same object within a DBMS, Doctrine's OCC is the best solution.
So it would be fine to explain it right here IMO.
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.
Indeed, a chapter about other altenatives would be relevant. I will add it when this PR will be merged given this PR had already been reviewed.
But in my opinion, it really depend of the use case:
for high concurrency use cases, atomic write operations are better when available.
True for concurrency between read and write, but wrong for concurrency between writes.
for low concurrency use cases, OCC is better when available.
True when computing the changset does not affect other systems:
- must be free (otherwise you pay twice to update the object) (ie. call to a billable API)
- must be quick (in a scalable environment, it's close to the previous point given you pay for the CPU time)
- other system can be rollbacked (ie. Don't call non-safe operation on third API, don't send email during the changset computation)
BTW, I am interested by what you said: we've spent lot of time and headaches
. Can you give me exemples/use case and how this component can be improved to fix such issue?
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.
True for concurrency between read and write, but wrong for concurrency between writes.
For both AFAIK. For example, when your business transaction simply consists in incrementing something in your data store, most DBMS (relational or not) does support this kind of atomic write. If several atomic writes are required, ACID-capable DBMS is required. This is applicable as long as the whole transaction can be executed on DBMS side.
True when computing the changset does not affect other systems:
Yes, not in all cases indeed, that's what I mean by saying "when available" (i.e. applicable).
BTW, I am interested by what you said: we've spent lot of time and headaches. Can you give me exemples/use case and how this component can be improved to fix such issue?
This component seems to be quite close from our own internal locking implementation. The main issue I've spotted (I may be wrong) is the one present here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Store/CombinedStore.php#L88 as described here symfony/symfony#22132 (review) (lack of onwership tracking to catch program flaws).
Issues can also arise at infrastructure level, and some hints may be added in the doc. For example, about the Redis setup, the user has to be aware of the HA vs consistency trade-off with this kind of backend. If consistency (lock must works as expected or not be working) is more important than availability (lock is always working, whatever its correctness) then a single Redis instance with maximum durability and without replication (even synchronous) should be preferred AFAIK.
Some other minor points:
- you provide a decorator which add blocking behavior to a non-blocking implementation, this looks great. But since this is implemented with a spinlock, the lack of fairness (no FIFO semantic) might be documented.
- I don't get what kind of problem solves CombinedStore. Some examples would be great.
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.
Issues can also arise at infrastructure level, and some hints may be added in the doc
I'm working on an other PR to add a lot of tips/warning about reliability and weakness of each store.
I don't get what kind of problem solves CombinedStore
Combine several stores together to provide an implementation of the Redlock pattern and offer an highest availability.
Hello guys. As Symfony 3.4 is released with the lock component... It could be a great idea to merge this PR. The link toward the doc is already available here but is broken: https://github.com/symfony/lock#resources |
Thank you @jderusse. |
This PR was merged into the 3.4 branch. Discussion ---------- [Lock] Re-add the Lock component in 3.4 This PR reverts #7865 as decided in symfony/symfony#22580 Commits ------- 9f1498d Add Lock component
This PR was merged into the 3.4 branch. Discussion ---------- [Lock] Integrate framework Should probably be merged after #7866 Documentation for symfony/symfony#22113 Commits ------- 142e703 Fixed minor issues 7f1e087 Add lock configuration in framework bundle
This PR reverts #7865 as decided in symfony/symfony#22580