[Lock] Check TTL expiration in lock acquisition #22542

jderusse · 2017-04-26T21:01:35Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22452
License	MIT
Doc PR	NA

This PR improve lock acquisition by throwing exception when the TTL is expired during the lock process.

nicolas-grekas · 2017-04-28T17:50:59Z

Can this wait 3.4? We'd like to close 3.3 asap :)

jderusse · 2017-04-28T17:58:35Z

From the 3 pending PR related to the components Lock, this one is the most important.

It avoid to acquire an expired Lock (due to network latency for instance). Cf discussion in the issue.

That's said, with an adequate warning in the documentation, this PR is not an absolute requirement.

jderusse · 2017-07-31T06:06:11Z

PR rebased and fixed add messages in Exceptions.

stloyd · 2017-07-31T07:02:55Z

src/Symfony/Component/Lock/Tests/Store/MemcachedStoreTest.php

+
+    public function testAbortAfterExpiration()
+    {
+        $this->markTestIncomplete('Memcached expecte a TTL greater than 1 sec. Simulating slow network is too complex');


This should be marked as skipped not incomplete.

fabpot · 2017-08-23T11:52:59Z

What's the status of this PR? I'd like to merge/close all PRs about the lock component ASAP to avoid having to revert the component for 3.4.

jderusse · 2017-08-23T12:03:45Z

@fabpot waiting for approvals. As saids in #22542 (comment) this PR is the most important IMO related to lock component.

fabpot · 2017-08-23T12:19:02Z

src/Symfony/Component/Lock/Store/CombinedStore.php


        foreach ($this->stores as $store) {
            try {
-                $store->putOffExpiration($key, $ttl);
+                if (0.0 >= $adjustedTtl = $expireAt - microtime(true)) {
+                    $this->logger->warning('TTL expires during the put off the expiration of the "{resource}" lock.', array('resource' => $key, 'store' => $store, 'ttl' => $ttl));


Message is not clear to me. Can you explain what you are trying to say?

We are trying to put off the expiration of the lock (to extend it life duration) but the method take more time to perform that task than the asked TTL.

For instance, when calling putOffExpiration($key, 31) takes 31 seconds.

fabpot · 2017-08-23T12:19:27Z

src/Symfony/Component/Lock/Store/CombinedStore.php

@@ -117,6 +124,10 @@ public function putOffExpiration(Key $key, $ttl)
            }
        }

+        if (microtime(true) >= $expireAt) {
+            throw new LockExpiredException(sprintf('Failed to put of the expiration the "%s" lock in less than "%f".', $key, $ttl));


If put off is the right verb, then there is a typo here.

missing of after expiration also

fabpot · 2017-08-23T12:19:41Z

src/Symfony/Component/Lock/Store/MemcachedStore.php

-            return;
+        $expireAt = microtime(true) + $this->initialTtl;
+        if (!$this->memcached->add((string) $key, $token, (int) ceil($this->initialTtl))) {
+            // the lock is already acquire. It could be us. Let's try to put off.


fabpot · 2017-08-23T12:20:22Z

src/Symfony/Component/Lock/Store/MemcachedStore.php

-        // the lock is already acquire. It could be us. Let's try to put off.
-        $this->putOffExpiration($key, $this->initialTtl);
+        if (microtime(true) >= $expireAt) {
+            throw new LockExpiredException(sprintf('Failed to store the "%s" lock in less than "%f".', $key, $this->initialTtl));


I would remove in less than. I would also add the unit.

removed (same for others usage)

fabpot · 2017-08-23T12:20:32Z

src/Symfony/Component/Lock/Store/MemcachedStore.php

@@ -105,6 +111,10 @@ public function putOffExpiration(Key $key, $ttl)
        if (!$this->memcached->cas($cas, (string) $key, $token, $ttl)) {
            throw new LockConflictedException();
        }
+
+        if (microtime(true) >= $expireAt) {
+            throw new LockExpiredException(sprintf('Failed to put of the expiration the "%s" lock in less than "%f".', $key, $ttl));


fabpot · 2017-08-23T12:20:43Z

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

            throw new LockConflictedException();
        }
+
+        if (microtime(true) >= $expireAt) {
+            throw new LockExpiredException(sprintf('Failed to put of the expiration the "%s" lock in less than "%f".', $key, $ttl));


fabpot · 2017-08-23T12:21:23Z

src/Symfony/Component/Lock/Tests/Store/MemcachedStoreTest.php

+
+    public function testAbortAfterExpiration()
+    {
+        $this->markTestSkipped('Memcached expecte a TTL greater than 1 sec. Simulating slow network is too complex');


typo expects
Simulating a slow network is too hard

jderusse · 2017-08-31T11:06:00Z

PR rebased with few changes

LockExpiredException does not extends LockConflictedException anymore because they are not related
Add a isExpired check in the lock::acquire and lock::refresh for more strength

chalasr

with minor comment

chalasr · 2017-08-31T11:48:41Z

src/Symfony/Component/Lock/Tests/Store/MemcachedStoreTest.php

+
+    public function testAbortAfterExpiration()
+    {
+        $this->markTestSkipped('Memcached expecte a TTL greater than 1 sec. Simulating a slow network is too hard');


typo expects

jderusse · 2017-08-31T14:05:00Z

failling test not related to this PR

jderusse · 2017-08-31T14:10:08Z

src/Symfony/Component/Lock/Key.php


-        if (null === $this->expiringDate || $newExpiringDate < $this->expiringDate) {


Switch to float comparison instead of datetime, due to bug in PHP => https://3v4l.org/bSt3d

fabpot · 2017-09-01T19:28:25Z

src/Symfony/Component/Lock/Key.php

-        if (null === $this->expiringDate || $newExpiringDate < $this->expiringDate) {
-            $this->expiringDate = $newExpiringDate;
+        if (null === $this->expiringTime || $this->expiringTime > $newTime) {
+            $this->expiringTime = $newTime;
        }
    }

    public function resetExpiringDate()


Should be resetExpiringTime() now, right?

fabpot · 2017-09-01T19:28:39Z

src/Symfony/Component/Lock/Key.php

        }
    }

    public function resetExpiringDate()
    {
-        $this->expiringDate = null;
+        $this->expiringTime = null;
    }

    /**
     * @return \DateTimeImmutable
     */
    public function getExpiringDate()


Using microtime is a internal implementation to fix issue with \Datetime comparison. IMHO there is no reason to change the public interface of this class, returning a \Datetime here is more usefull than returning a microtime.

If I have to change it, I would rather stop using ExpiringThing and replace them by lifetime with resetLifetime(), reduceLifetime(float $ttl), isExpired(): bool and getTimeToLive(): float WDYT?

We now have a public method that takes a float and the rest of the API dealing with DateTime, that feels slightly inconsistent to me as well. I'm fine with float only.
Would getTimeToLive() return the remaining ttl at the moment? Or just the original (full) ttl?
Do we really need it? (just wondering)

getTimeToLive should returns the raimaining TTL. Returning the initial TTL without knowing when this TTL had been sets is useless.

The purpose is to let the locker knowing if he has time to perform his job.
For instance you iterate over thousands of items. You know that it takes between 1 and 3 seconds to treat 1 item. You can use the the getTimeToLive after each item and when the TTL is below 5 seconds you call the refresh() method to extends the life of your lock.

Sounds good to me

jderusse · 2017-09-01T22:36:07Z

src/Symfony/Component/Lock/Key.php

    {
-        $this->expiringDate = null;
+        return null === $this->expiringTime ? null : $this->expiringTime - microtime(true);


Should it be null or (float) PHP_MAX_INT?

I'd keep null with a comment explaining its meaning in the return annotation

chalasr · 2017-09-03T12:09:22Z

👍

fabpot · 2017-09-03T14:31:52Z

Thank you @jderusse.

…sse) This PR was squashed before being merged into the 3.4 branch (closes #22542). Discussion ---------- [Lock] Check TTL expiration in lock acquisition | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22452 | License | MIT | Doc PR | NA This PR improve lock acquisition by throwing exception when the TTL is expired during the lock process. Commits ------- f74ef35 [Lock] Check TTL expiration in lock acquisition

carsonbot added Status: Needs Review Feature labels Apr 26, 2017

jderusse changed the title ~~Check TTL expiration in lock acquisition~~ [Lock] Check TTL expiration in lock acquisition Apr 26, 2017

jderusse force-pushed the lock-ttl-check branch from 7bd2873 to 3b0db33 Compare April 26, 2017 22:07

nicolas-grekas added this to the 3.3 milestone Apr 27, 2017

nicolas-grekas modified the milestones: 3.3, 3.4 Apr 29, 2017

nicolas-grekas changed the base branch from master to 3.4 May 23, 2017 16:48

jderusse force-pushed the lock-ttl-check branch from 3b0db33 to c09e7b6 Compare July 31, 2017 05:58

stloyd reviewed Jul 31, 2017

View reviewed changes

jderusse force-pushed the lock-ttl-check branch from c09e7b6 to 76587d0 Compare July 31, 2017 11:26

chalasr added the Lock label Jul 31, 2017

fabpot requested changes Aug 23, 2017

View reviewed changes

jderusse force-pushed the lock-ttl-check branch from 76587d0 to 0bd266a Compare August 23, 2017 13:01

jderusse force-pushed the lock-ttl-check branch 4 times, most recently from 54e28aa to aa6c353 Compare August 31, 2017 11:02

jderusse force-pushed the lock-ttl-check branch from aa6c353 to 1522312 Compare August 31, 2017 11:19

chalasr approved these changes Aug 31, 2017

View reviewed changes

jderusse force-pushed the lock-ttl-check branch 2 times, most recently from a7bc204 to 2cd518e Compare August 31, 2017 12:46

jderusse force-pushed the lock-ttl-check branch 4 times, most recently from 81162a6 to 2e4f434 Compare August 31, 2017 13:56

Check TTL expiration in lock acquisition

9743bfb

jderusse force-pushed the lock-ttl-check branch from 2e4f434 to 9743bfb Compare August 31, 2017 14:04

jderusse commented Aug 31, 2017

View reviewed changes

fabpot requested changes Sep 1, 2017

View reviewed changes

jderusse force-pushed the lock-ttl-check branch from 1d60277 to 3b6be16 Compare September 1, 2017 22:34

jderusse commented Sep 1, 2017

View reviewed changes

jderusse force-pushed the lock-ttl-check branch from 3b6be16 to cb7b16c Compare September 3, 2017 10:18

Use lifetime instead of ExpiringDate

b9de71b

jderusse force-pushed the lock-ttl-check branch from cb7b16c to b9de71b Compare September 3, 2017 10:23

fabpot approved these changes Sep 3, 2017

View reviewed changes

fabpot closed this Sep 3, 2017

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

jderusse deleted the lock-ttl-check branch August 2, 2019 12:17


		if (null === $this->expiringDate \|\| $newExpiringDate < $this->expiringDate) {

[Lock] Check TTL expiration in lock acquisition #22542

[Lock] Check TTL expiration in lock acquisition #22542

Conversation

jderusse commented Apr 26, 2017 • edited by chalasr Loading

nicolas-grekas commented Apr 28, 2017

jderusse commented Apr 28, 2017

jderusse commented Jul 31, 2017

Choose a reason for hiding this comment

fabpot commented Aug 23, 2017

jderusse commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jderusse commented Aug 31, 2017 • edited Loading

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jderusse commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Sep 3, 2017

fabpot commented Sep 3, 2017

jderusse commented Apr 26, 2017 •

edited by chalasr

Loading

jderusse commented Aug 31, 2017 •

edited

Loading