10000 [Lock] Consolidate enforcement of Lock expiry · Issue #28779 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Lock] Consolidate enforcement of Lock expiry #28779

New issue
8000

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Closed
kralos opened this issue Oct 8, 2018 · 4 comments
Closed

[Lock] Consolidate enforcement of Lock expiry #28779

kralos opened this issue Oct 8, 2018 · 4 comments
Labels
Enhancement Lock Stalled

Comments

@kralos
Copy link
kralos commented Oct 8, 2018 •
edited
Loading

Description
Enforcement of Lock\Key expiry is handled both by all of the Lock\Store\* as well as Lock\Lock. In the interest of DRY and maintaining consistent behavior, we should move enforcement to exist in Lock\Lock only.

Example

diff --git a/src/Symfony/Component/Lock/Store/MemcachedStore.php b/src/Symfony/Component/Lock/Store/MemcachedStore.php
index c9200af199..dd92024288 100644
--- a/src/Symfony/Component/Lock/Store/MemcachedStore.php
+++ b/src/Symfony/Component/Lock/Store/MemcachedStore.php
@@ -63,10 +63,6 @@ class MemcachedStore implements StoreInterface
             // the lock is already acquired. It could be us. Let's try to put off.
             $this->putOffExpiration($key, $this->initialTtl);
         }
-
-        if ($key->isExpired()) {
-            throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
-        }
     }
 
     public function waitAndSave(Key $key)
@@ -109,10 +105,6 @@ class MemcachedStore implements StoreInterface
         if (!$this->memcached->cas($cas, (string) $key, $token, $ttl)) {
             throw new LockConflictedException();
         }
-
-        if ($key->isExpired()) {
-            throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
-        }
     }
 
     /**

Please See Lock\Lock::acquire and Lock\Lock::refresh:

if ($this->key->isExpired()) {
    throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $this->key));
}
if ($this->key->isExpired()) {
    throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $this->key));
}
@kralos kralos mentioned this issue Oct 8, 2018
Merged
@jderusse
Copy link
Member

The Lock class is a facade. Stores can be used directly.

IMHO If we want to refactor that part, it should be removed from the Lock class and done on Stores.

@kralos kralos changed the title [Lock] Move enforcement of Key expiry to Lock [Lock] Consolidate enforcement of Lock expiry Mar 28, 2019
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Friendly ping? Should this still be open? I will close if I don't hear anything.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement Lock Stalled
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jderusse @kralos @xabbuh @carsonbot
0