[Lock] Scoped Lock #22115

fbourigault · 2017-03-22T21:40:43Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	soon

This add to the just-born Lock component a scoped lock.

A scoped lock is a lock which will get automatically released when destructed.

Usage

class MyService
{
    private $lockFactory;

    public function __construct(LockFactoryInterface $lockFactory)
    {
        $this->lockFactory = $lockFactory;
    }

    public function run()
    {
        $lock = $this->lockFactory->createScopedLock(__METHOD__);
        $lock->acquire();

       // some locked job

        if (/* early return test*/) {
            return; //the lock will be released when leaving the function scope.
        }
        
        try {
             //some other locked job
        } catch (\Exception $e) {
             //no need to release the lock. it will be automatically released when leaving the function scope.
        }
        
    } //lock is automatically released here
}

fabpot · 2017-03-22T21:45:29Z

ping @jderusse

fbourigault · 2017-03-22T21:48:17Z

Do we include the documentation in the main component PR, or I create a new one based on the first one?

fbourigault · 2017-03-22T21:56:26Z

Tests are failing for php 7.1 with lowest dependencies. It does not looks related to my PR https://travis-ci.org/symfony/symfony/jobs/214019847#L3293-L3323.

jderusse · 2017-03-22T22:28:35Z

Trying the solve test failure in #22119

jderusse · 2017-03-23T00:33:14Z

LGTM.
This is a clever idea, and would simplify several use case of this component.

robfrawley · 2017-03-23T01:55:12Z

While I generally think this is a great feature, there are some caveats to relying on the destructor for releasing locks where the Lock Component's destructor will not be called, causing the lock to persist, specifically:

If throw is called in another destructor
If exit is called in another destructor
If exit is called from a shutdown function registered via register_shutdown_function (depending on PHP version, it seems)
If there is a fatal error

The lock will persist in all the above, and possibly other, cases. We should be clear about these caveats in the documentation for this PR, IMHO.

jderusse · 2017-03-23T02:05:26Z

Stores are made to avoid to keep a lock for ever (segfault and brutal interrupt are handled) thank to the TTL mechanism (but it's not perfect)

But you're right. We can add to the list:

if the variable is copied (stored in a kind of cache or whatever)
after a circular reference assignment of the lock variable, we don't know when the GC will call the destructor

fbourigault · 2017-03-23T05:51:55Z

@robfrawley, all cases you listed are not specific to the scoped lock. So like @jderusse said, you can only rely on the lock timeout.

What about a warning in the scoped lock dock about the two points @jderusse listed?

greg0ire

Very nice contribution!

greg0ire · 2017-03-23T09:30:57Z

src/Symfony/Component/Lock/Factory.php

+     */
+    public function createScopedLock($resource, $ttl = 300.0)
+    {
+        $lock = $this->createLock($resource, $ttl);


Thoughtless OTV, consider removing it.

greg0ire · 2017-03-23T09:31:12Z

src/Symfony/Component/Lock/Factory.php

+     * Create a scoped lock for the given resource.
+     *
+     * @param string $resource The resource to lock
+     * @param float  $ttl      maximum expected lock duration


In what unit? Please document that.

greg0ire · 2017-03-23T09:31:56Z

src/Symfony/Component/Lock/ScopedLock.php

+namespace Symfony\Component\Lock;
+
+/**
+ * ScopedLock encapsulate a LockInterface which is automatically released when destructed.


"encapsulateS"

greg0ire · 2017-03-23T09:32:54Z

src/Symfony/Component/Lock/ScopedLock.php

+    }
+
+    /**
+     * Automatically release the underlying lock when object is destructed.


"when the object"

fbourigault · 2017-03-23T09:56:53Z

Broken tests are addressed in #22122

fbourigault · 2017-03-23T12:12:34Z

@greg0ire I addressed all changes you requested.
I also added parameters description for Lock.

greg0ire

👌

robfrawley · 2017-03-23T14:06:42Z

@fbourigault I just think we should mention these caveats in the documentation PR, is all.

nicolas-grekas · 2017-03-23T15:04:18Z

I'm 👎 here. This makes releasing totally implicit. That's a foot gun to me.

HHVM doesn't have the same destructing rules than PHP (they can even be not called)
when a custom error handler keeps the $context, the lock will not be released
same in the cases listed by @robfrawley (gc, stack traces, etc.)

Adding a decorator to provide an unreliable behavior looks like... unreliable, thus my stand.
If you want that, but in a predictable way, just wrap in a try+finally block.

fabpot · 2017-03-23T21:57:49Z

Closing for the reasons explained by @nicolas-grekas

…jderusse) This PR was squashed before being merged into the 3.4 branch (closes #22132). Discussion ---------- [Lock] Automaticaly release lock when user forget it | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | The objective of this PR is to automatically release the Lock when the user forget to call the `release` method (like the `fclose` function does in, php core). The implementation is comparable to #22115 but the purpose is not the same. I create a new PR to not mix things. Commits ------- 2eedc1f [Lock] Automaticaly release lock when user forget it

carsonbot added Status: Needs Review Feature labels Mar 22, 2017

greg0ire suggested changes Mar 23, 2017

View reviewed changes

greg0ire approved these changes Mar 23, 2017

View reviewed changes

add a scoped lock implementation

6f00ad2

jderusse mentioned this pull request Mar 23, 2017

[Lock] Automaticaly release lock when user forget it #22132

Closed

fabpot closed this Mar 23, 2017

fbourigault deleted the scoped-lock branch September 17, 2018 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lock] Scoped Lock #22115

[Lock] Scoped Lock #22115

fbourigault commented Mar 22, 2017 •

edited

Loading

fabpot commented Mar 22, 2017

fbourigault commented Mar 22, 2017

fbourigault commented Mar 22, 2017

jderusse commented Mar 22, 2017

jderusse commented Mar 23, 2017

robfrawley commented Mar 23, 2017 •

edited

Loading

jderusse commented Mar 23, 2017

fbourigault commented Mar 23, 2017

greg0ire left a comment

greg0ire Mar 23, 2017

greg0ire Mar 23, 2017

greg0ire Mar 23, 2017

greg0ire Mar 23, 2017

fbourigault commented Mar 23, 2017

fbourigault commented Mar 23, 2017

greg0ire left a comment

robfrawley commented Mar 23, 2017

nicolas-grekas commented Mar 23, 2017

fabpot commented Mar 23, 2017

[Lock] Scoped Lock #22115

[Lock] Scoped Lock #22115

Conversation

fbourigault commented Mar 22, 2017 • edited Loading

Usage

fabpot commented Mar 22, 2017

fbourigault commented Mar 22, 2017

fbourigault commented Mar 22, 2017

jderusse commented Mar 22, 2017

jderusse commented Mar 23, 2017

robfrawley commented Mar 23, 2017 • edited Loading

jderusse commented Mar 23, 2017

fbourigault commented Mar 23, 2017

greg0ire left a comment

Choose a reason for hiding this comment

greg0ire Mar 23, 2017

Choose a reason for hiding this comment

greg0ire Mar 23, 2017

Choose a reason for hiding this comment

greg0ire Mar 23, 2017

Choose a reason for hiding this comment

greg0ire Mar 23, 2017

Choose a reason for hiding this comment

fbourigault commented Mar 23, 2017

fbourigault commented Mar 23, 2017

greg0ire left a comment

Choose a reason for hiding this comment

robfrawley commented Mar 23, 2017

nicolas-grekas commented Mar 23, 2017

fabpot commented Mar 23, 2017

fbourigault commented Mar 22, 2017 •

edited

Loading

robfrawley commented Mar 23, 2017 •

edited

Loading