Skip to content

[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

Merged
merged 1 commit into from
Dec 7, 2017
Merged

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented May 1, 2017

This PR reverts #7865 as decided in symfony/symfony#22580

@xabbuh xabbuh added the Lock label May 1, 2017
@xabbuh xabbuh added this to the 3.4 milestone May 1, 2017
@xabbuh xabbuh added the On hold label May 1, 2017
The Lock Component creates and manages `locks`_, a mechanism to provide
exclusive access to a shared resource.

.. versionadded:: 3.3
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch @apfelbox

@jderusse
Copy link
Member Author

Component re-added (symfony/symfony#22597)
Time to re-merge this PR?

I'll create a separate PR to fix issue related to issues

@jderusse jderusse changed the base branch from master to 3.4 June 19, 2017 21:27
@HeahDude HeahDude removed the On hold label Jul 29, 2017
@jderusse
Copy link
Member Author

Can we move forward with the Lock documentation ?

I wish to improve the documentation to fix symfony/symfony#22452
and prefere to do it in a separated PR instead of updating this one (because it has already been merged once).

@HeahDude
Copy link
Contributor

Ok for me. @xabbuh?

This reverts commit e14709d.
$lock = $factory->createLock('pdf-invoice-generation');

if ($lock->acquire()) {
// The resource "pdf-invoice-generation" is locked.

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;
}

Copy link
Member Author

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
Copy link

@NoiseByNorthwest NoiseByNorthwest Sep 10, 2017

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.

Copy link
Member Author

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?

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.

Copy link
Member Author

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.

@Nek-
Copy link
Contributor

Nek- commented Dec 7, 2017

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

@xabbuh
Copy link
Member

xabbuh commented Dec 7, 2017

Thank you @jderusse.

@xabbuh xabbuh merged commit 9f1498d into symfony:3.4 Dec 7, 2017
xabbuh added a commit that referenced this pull request Dec 7, 2017
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
javiereguiluz added a commit that referenced this pull request Jan 29, 2018
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
@jderusse jderusse deleted the lock-34 branch April 18, 2020 10:09
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.

7 participants