Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Lock] Create a lock component#21093

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
jderusse wants to merge4 commits intosymfony:masterfromjderusse:lock
Closed

Conversation

@jderusse
Copy link
Member

@jderussejderusse commentedDec 29, 2016•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?they will
Fixed tickets#20382
LicenseMIT
Doc PRsymfony/symfony-docs#7364

This PR aim to add a new component Lock going further than the FileSystem\LockHandler by allowing remote backend (like Redis, memcache, etc)
Inspired by

Usage

The simplest way to use lock is to inject an instance of a Lock in your service

class MyService{private$lock;publicfunction__construct(LockInterface$lock)    {$this->lock =$lock;    }publicfunctionrun()    {$this->lock->acquire(true);// If I'm here, no exception had been raised. Lock is acquiredtry {// do my job        }finally {$this->lock->release();        }    }}

Configured with something like

services:app.my_service:class:AppBundle\MyServicearguments:            -app.lock.my_serviceapp.lock.my_service:class:Symfony\Component\Lock\Lockfactory:['@locker', createLock]arguments:['my_service']

If you need to lock serveral resource on runtime, wou'll nneed to inject the LockFactory.

class MyService{private$lockFactory;publicfunction__construct(LockFactoryInterface$lockFactory)    {$this->lockFactory =$lockFactory;    }publicfunctionrun()    {foreach ($this->itemsas$item) {$lock =$this->lockFactory->createLock((string)$item);try {$lock->acquire();            }catch (LockConflictedException$e) {continue;            }// When I'm here, no exception had been, raised. Lock is acquiredtry {// do my job            }finally {$lock->release();            }        }    }}

Configured with something like

services:app.my_service:class:AppBundle\MyServicearguments:            -'@locker'

This component allow you to refresh an expirable lock.
This is usefull, if you run a long operation split in several small parts.
If you lock with a ttl for the overall operatoin time and your process crash, the lock will block everybody for the defined TTL.
But thank to the refresh method, you're able to lock for a small TTL, and refresh it between each parts.

class MyService{private$lock;publicfunction__construct(LockInterface$lock)    {$this->lock =$lock;    }publicfunctionrun()    {$this->lock->acquire(true);try {do {$finished =$this->performLongTask();// Increase the expire date by 300 more seconds$this->lock->refresh();            }while (!$finished)// do my job        }finally {$this->lock->release();        }    }}

Naming anc implementation choise

$lock->acquire()vs$lock->lock()

Choose to use acquire, because this component is full oflock Symfony\Component\Lock\Lock::Lock` raised a E_TOO_MANY_LOCK in my head.

$lock->acquire(false);$lock->acquire(true);vs$lock->aquire()$lock->waitAndAquire()

Not a big fan of flag feature and 2. But I choose to use the blocking flag to offer a simple (and common usecase) implementation

$lock = $factory->createLock($key);$lock->acquire();vs$lock->aquire($key)

I choose to a the pool of locks implementation. It allow the user to create 2 instances and use cross lock even in the same process.

interface LockInterface final class Lock implements LockInterfacevsfinal class Lock

I choose to use a Interface even if there is only one implementaiton to offer an extension point here

TODO

In this PR

  • tests
  • add logs
  • offer several redis connectors
  • try other store implementation to validate the architecture/interface

In other PR

  • documentation
  • add configuration in framework bundle
  • add stop watch in the debug bar
  • improve the combined store (takes the drift into account and elapsed time between each store)
  • implement other stores (memcache, ...)
  • use this component in session manipulation (fixes[HttpFoundation] Session handlers should use a lock #4976)

andersonamuller, maidmaid, jjsaunier, linaori, sstok, Nicofuma, and fnash reacted with thumbs up emoji
@lyrixx
Copy link
Member

lyrixx commentedDec 29, 2016•
edited
Loading

When I worked on the Filesystem/LockHandler we (fab, niko, me) decided to not implement a "lock component" because it was always very specific to each use case: one vs multiple server. lock in redis with a ttl, Redlock, etc.

May be it's time to reconsider this and to introduce in symfony a new lock component.

Anyway, about the end user API:

I saw you arguments aboutaquire vslock and for what is worth is preferlock andrelease.

More over, I would prefer a system based onfalse/true than on exception.

@andersonamuller
Copy link
Contributor

Since you said the component is already full of Lock names, what about call the componentMutex?

mnapoli reacted with thumbs up emoji

Copy link
Contributor

@linaorilinaori left a comment

Choose a reason for hiding this comment

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

Some minor points, architectural I think it looks fine. I'm not too experienced with locking itself, but this component is similar an in-house Mutex component my company wrote, thus I do like it a lot.

throw $e;
} catch (\Exception $e) {
throw new LockAcquiringException('', 0, $e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you be catching\Throwable as well?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I wonder if it make sens to wrap every exception with "LockException" here?
Or I should just let the original exception been thrown.

If yes, you're right, I should catch everything.
If no, I can remove this statement.

WDYT?

throw $e;
} catch (\Exception $e) {
throw new LockAcquiringException('', 0, $e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you be catching\Throwable as well?

sstok reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-grekasDec 29, 2016•
edited
Loading

Choose a reason for hiding this comment

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

on Error, it should let the error throw imho. throwing a LockAcquiringException would make no sense. Error is a dev mistake, not a runtime one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I wanted to make sure this was considered

} else {
throw new InvalidArgumentException(
sprintf('Unable to acquire a blocking lock on a instance of "%s".', get_class($this->store))
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go on 1 line

ro0NL and sstok reacted with thumbs up emoji
*
* @author Jérémy Derussé <jeremy@derusse.com>
*/
class CombinedStore implements StoreInterface, ExpirableStoreInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be made final

*
* @author Jérémy Derussé <jeremy@derusse.com>
*/
class UnanimousQuorum implements QuorumInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be made final

use Symfony\Component\Lock\Store\ExpirableStoreInterface;

/**
* Lock is the default implementation to the LockInterface.
Copy link
Contributor

Choose a reason for hiding this comment

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

implementationto of the LockInterface

{
/**
* Acquire the lock. If the lock is acquired by someone else, the parameter `blocking` determine whether or not
* the the call should block until the release of the lock.
Copy link
Contributor

Choose a reason for hiding this comment

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

the parameterblocking determines whether

* Acquire the lock. If the lock is acquired by someone else, the parameter `blocking` determine whether or not
* the the call should block until the release of the lock.
*
* @param bool $blocking whether or not the Lock should wait the release of someone else
Copy link
Contributor

Choose a reason for hiding this comment

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

should waitfor the release of

* Returns Whether or not the quorum *could* be met.
* This method does not means the quorum *would* be met for sur. But can be useful to early stop a process when you
* known there isn't any chance to met the quorum.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be a blank line between the main and sub description

This method does not means the quorum *would* be met forsur. But sure, but can be useful toearly stop a processearly when you known thereisn't anyis no chance to meet the quorum.

interface BlockingStoreInterface
{
/**
* Wait a key became free then stores the resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait a keybecamebecomes free**,** then stores the resource.

Copy link
Contributor

@ro0NLro0NL left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Agree on the finals. Im not into locking myself as well.. at first it looked slightly over-engineered.. then again i could understand we're super flexible here in terms of supporting various complex strategies.

I think i'd prefertrue/false API as well.

And not sure.. but cant we do closures somehow to streamline the process?

public function __construct(StoreInterface $decorated, $retrySleep = 50, $retryCount = PHP_INT_MAX)
{
$this->decorated = $decorated;
$this->retryCount = $retryCount;
Copy link
Contributor

@ro0NLro0NLDec 29, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Perhaps use$retryCount ?: PHP_INT_MAX and go0 in the construct. To keep up with the promise like@iltar mentioned 👍

@javiereguiluz
Copy link
Member

Regarding the naming of the methods, I've been checking the practices of other languages:

Python:

lock=Lock()lock.acquire()# waits if lock is already heldlock.acquire(False)# doesn't wait if lock is already heldlock.release()iflock.locked()

Ruby:

lock=Mutex.newlock.lock# waits if lock is already heldlock.try_lock# doesn't wait if lock is already heldlock.unlockiflock.locked?

PHP pthreads extension

$lock = Mutex::create();Mutex::lock($lock);// waits if lock is already heldMutex::trylock($lock);// doesn't wait if lock is already heldMutex::unlock($lock);// no method to check if the lock is locked

Java:

Locklock = ...;lock.lock();// waits if lock is already heldlock.tryLock();// doesn't wait if lock is already heldlock.unlock();// no method to check if the lock is locked

It looks like the most common choice is:lock,tryLock andunlock

ro0NL, linaori, yceruto, grachevko, maximecolin, Wirone, and fnash reacted with thumbs up emoji

*/
public function generateToken()
{
return base64_encode(random_bytes($this->entropy / 8));

Choose a reason for hiding this comment

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

I suggest inlining this and removeRandomTokenGenerator andTokenGeneratorInterface: they are just internal details.

*/
public function createLock($resource)
{
return new Lock(new Key($resource), $this->store);

Choose a reason for hiding this comment

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

what about removing the factory and move createLock to the store interface?

);

// Silence error reporting
set_error_handler(function () {

Choose a reason for hiding this comment

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

single line

Copy link
Member

Choose a reason for hiding this comment

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

indeed

return;
}

$fileName = sprintf(

Choose a reason for hiding this comment

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

single line

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Is there a limit to the line length in CS?

@jderusse
Copy link
MemberAuthor

jderusse commentedDec 29, 2016•
edited
Loading

  • Moved the createLock factory in the StoreInterface
    Create an AbstractClass to avoid dupplication of code (Could be a trait too, WDYT)
  • Remove the TokenGenerator

API choices:

IMO lock and unlock fit well when the subject is the resource. ie.$resource->lock() or$handler->lock($resource);
In this PR the subject is not the resource, but the Lock itself, in this case, I think the python naming is meaningful$lock->acquire/release.

I like the split in lock/tryLock, I think I'll apply this change unless someone prefer the $blocking flag?

About the return true/false vs \Exception. Personally, I see it like "lock me the resource right now" instead of "can you please lock the resource". If the lock is not acquired, it means sometinhg goes wrong, that's why I excpect an exception.
But that's just my taste/opinion

andersonamuller and Nicofuma reacted with thumbs up emoji

$this->resource = (string) $resource;
}

public function getResource()
Copy link
Member

Choose a reason for hiding this comment

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

__toString()?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

good idea

}

/**
* @return mixed
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed as it does not say anything useful

$this->store->expire($this->key, $ttl);
$this->logger->info('Expiration defined for lock "{resource}" for "{ttl}" seconds', array('resource' => $this->key->getResource(), 'ttl' => $ttl));
} catch (LockConflictedException $e) {
$this->logger->info('Fail to define a expiration for the lock "{resource}". Someone else acquired the lock', array('resource' => $this->key->getResource()));
Copy link
Member

Choose a reason for hiding this comment

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

an expiration, same below.

Copy link
Member

Choose a reason for hiding this comment

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

I would use a single sentence here instead of 2.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, I don't see how to merge this two sentences without removing a part (which are both important IMO) :(

Failed to define an expiration for the "{resource}" lock because someone else acquired it?

$this->logger->info('Fail to define a expiration for the lock "{resource}". Someone else acquired the lock', array('resource' => $this->key->getResource()));
throw $e;
} catch (\Exception $e) {
$this->logger->info('Fail to define a expiration for the lock "{resource}"', array('resource' => $this->key->getResource(), 'exception' => $e));
Copy link
Member

Choose a reason for hiding this comment

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

the {resource} lock.

interface LockInterface
{
/**
* Acquire the lock. If the lock is acquired by someone else, the parameter `blocking` determines whether or not
Copy link
Member

Choose a reason for hiding this comment

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

Acquires, same everywhere else.

interface QuorumInterface
{
/**
* Returns Whether or not the quorum is met.
Copy link
Member

Choose a reason for hiding this comment

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

extra cap (same below)

);

// Silence error reporting
set_error_handler(function () {
Copy link
Member

Choose a reason for hiding this comment

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

indeed


/**
* RetryTillSaveStore is a StoreInterface implementation which decorate a non blocking StoreInterface to provide a
* kind of blocking storage.
Copy link
Member

Choose a reason for hiding this comment

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

"a kind of" should probably be removed.

public function exists(Key $key);

/**
* Retrieves a lock for the given resource.
Copy link
Member

Choose a reason for hiding this comment

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

It retrieves or it creates?

/**
* {@inheritdoc}
*/
public function acquire($blocking = false, $ttl = 300.0)
Copy link
Member

Choose a reason for hiding this comment

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

What about moving the defaultttl value to the constructor instead?

ro0NL reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The purpose of this parameter was to let the user choose a specific duration for one call.

Given the easiest (and probably the most used) workflow would be to use theStore to create theLock, it does not make sens to move this parameter in the __constructor: If we want a fixed ttl value, we can just remove it and let the store use what he want.

I'll replace the default value300.0 by anull. It let the user to override the default store configuration.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Finally move the parameter in the constructor (as suggested) and remove it from the refresh method too (because this method serves the same purpose).

/**
* {@inheritdoc}
*/
public function refresh($ttl = 300.0)
Copy link
Member

Choose a reason for hiding this comment

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

I would make thettl argument required, without a default value.

ro0NL reacted with thumbs up emoji

return;
} catch (LockConflictedException $e) {
usleep($this->retrySleep * 1000);
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Should add randomness to avoid eternal collision

namespace Symfony\Component\Lock\Exception;

/**
* InvalidArgumentException is thrown when a service had been badly initialized.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it has a specific usage, the name should reflect it.

@jderusse
Copy link
MemberAuthor

Pushed a new commit to remove the 2 interfacesBlockingStoreInterface andExpirableStoreInterface and merge the methods intoStoreInterface.

At first it was a god thing to split it 3 interfaces to separate responsibilities, but I realize that it was a pain to decorate the stores because we both need to implement every interfaces and are not sure that the decorated store will implement every interfaces. In this case, the decorator don't know what to do.

@nicolas-grekasnicolas-grekas added this to the3.x milestoneJan 2, 2017
@fabpot
Copy link
Member

@jderusse you need to also add the new component in thereplace key of the rootcomposer.json file.

@@ -0,0 +1,19 @@
Copyright (c) 2016 Fabien Potencier
Copy link
Member

Choose a reason for hiding this comment

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

should be updated to2016-2017 I suppose

@jderussejderusseforce-pushed thelock branch 2 times, most recently from816523b tof5b20bdCompareJanuary 10, 2017 23:17
*
* @author Jérémy Derussé <jeremy@derusse.com>
*/
final class Lock implements LockInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not implementLoggerAwareInterface as well?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

fixed

*/
public function createLock($resource, $ttl = 300.0)
{
return new Lock(new Key($resource), $this, $ttl);
Copy link
Member

Choose a reason for hiding this comment

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

making stores create the lock by passing$this to the Lock instance makes it very hard to use composition because the Lock instance must always be created by theoutside store in this case instead of delegating the call tocreateLock (otherwise, the lock has the wrong store, as it ends up calling directly the inner store).

IMO, the Store should not be the lock factory at the same time. Mixing these responsibilities is what makes things hard.

We should either have a separate factory (in which the store is injected, and so decoration works fine as the factory would get the outside store) or let the instantiation be done in userland when using the library.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

That was the first implementation (I like the separation of responsibility too).
But, as suggested by@nicolas-grekas inthis comment I move the factory into the StoreInterface and that's why I create the AbstractStore to avoid code duplication (which I don't really like too).

The issue with the factory is was an empty class like without real value:

class StoreFactory{private$store;publicfunction__construct(StoreInterface$store)    {$this->store =$store;    }publicfunctioncreateLock($resource,$ttl =300.0)    {returnnewLock(newKey($resource),$this->lock,$ttl);    }}

And IMO, the outer store may create the lock instead of delegating the creation to the inner store. The abstractStore do the job (maybe a trait could be a better solution than the abstraction)

What do you think ?

(just to be sure, you are talking about decoration instead of composition ?)

try {
$store->delete($key);
} catch (\Exception $e) {
} catch (\Throwable $e) {
Copy link
Member

Choose a reason for hiding this comment

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

catching all exceptions silently would be a debugging nightmare

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed, but with a notice level given the deletion failure may not be an issue

$store->putOffExpiration($key, $ttl);
++$successCount;
} catch (\Exception $e) {
++$failureCount;
Copy link
Member

Choose a reason for hiding this comment

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

we are loosing all exceptions messages here, making debugging really hard

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed


/**
* Tests blocking locks thanks to pcntl.
*
Copy link
Member

Choose a reason for hiding this comment

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

missing@requires extension pcntl

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I wasn't aware of this feature. Thanks. I also annotate \Redis tests

$drift = 20000;

if (PHP_VERSION_ID < 50600 || defined('HHVM_VERSION_ID')) {
$this->markTestSkipped('Php 5.5 does not keep resource in child forks');
Copy link
Member

Choose a reason for hiding this comment

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

message is wrong in the case of HHVM

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

fixed


public function getStore()
{
$redis = new \Predis\Client('tcp://'.getenv('REDIS_HOST').':6379');
Copy link
Member

Choose a reason for hiding this comment

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

what if there is no such env variable ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This part of the test is mostly a copy/paste of the RedisAdapterTest from the Cache component.
The variable is define is thephpunit.xml file and should exists.

I don't think it worst it to check whether or not someone destroy the config file and expect a successful result.

@jjsaunier
Copy link

jjsaunier commentedJan 11, 2017•
edited
Loading

I think a little explanation about clock drift (which is introduced) could be a plus, a link in code comment, or in the docs to automatically answer about the "why" when people look at the code could be useful:)

@jderusse
Copy link
MemberAuthor

@Prophet777 You're absolutely right, but this PR don't yet introduce a clock drift imlplementation.

I'll do it in a next PR or in a other PR

Copy link
Contributor

@WironeWirone left a comment

Choose a reason for hiding this comment

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

My 2 (literally) cents.

*/
public function putOffExpiration(Key $key, $ttl)
{
// do nothing, the flock locks forever.
Copy link
Contributor

Choose a reason for hiding this comment

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

Invalid comment, probably copy-pasted.

}

// Have to be a \Predis\Client
return call_user_func_array(array($this->redis, 'eval'), array_merge(array($script, 1, $resource), $args));
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that constructor allows only some classes and most of them are used above, but wouldn't be more readable to check instance of\Predis\Client too? After allif statements exception could be thrown that script could not be evaluated.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed in#22117
Thank you@Wirone

@goetas
Copy link
Contributor

Probably I'm late, but in the past I've worked with an almost identical lock model, and it almost never worked.

If the process responsible for releasing the lock dies, most of the time the lock needs to be released manually (deleting the lock file or the redis record). The timeout mitigates the problem but does not solve it.

{
$this->store->delete($this->key);

if ($this->store->exists($this->key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In a concurrent environment, another process might have created the same lock here.exists will return true even if the previousdelete was successful.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

If the lock had been acquired by an other instance ofkey, the store will treat them like 2 different keys and will return false.

goetas reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i got it. there is a "token" in between, that is always random, for the samekey you have different "tokens"

* @throws LockConflictedException If the lock is acquired by someone else in blocking mode
* @throws LockAcquiringException If the lock can not be acquired
*/
public function acquire($blocking = false);
Copy link
Contributor

@goetasgoetasMar 24, 2017•
edited
Loading

Choose a reason for hiding this comment

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

To avoid concurrency issues this should return a unique reference for the lock, so the release should work by using this reference

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This unique reference is in the key. By passing the key to the methods save/delete/exists the concurrency is already handled

goetas reacted with thumbs up emoji
@jocel1
Copy link

jocel1 commentedMar 24, 2017•
edited
Loading

What about adding a MysqlStore using GET_LOCK() / RELEASE_LOCK() / IS_FREE_LOCK() functions ?
https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_get-lock

It's robust and available in all MySQL versions.

@jderusse
Copy link
MemberAuthor

@goetas Then I've got a good news to you, the actual implementation does not suffer for such issue. Thanks toflock (or equivalent mechanism in other stores) the system kernel will automatically free the lock even if you forget to release it, or die, or segfault, ...

@jocel1 I didn't dig into MySQL store, but it's on my todo list. Do you want to create that PR?

@goetas
Copy link
Contributor

@jocel1 In mysql there is a fundamental concept that PHP has not, the connection.

As the documentation says:

A lock obtained with GET_LOCK() is released explicitly by executing RELEASE_LOCK() or implicitly when your session terminates (either normally or abnormally)

At the end of the connection, the lock will be automatically released.

@jocel1
Copy link

@goetas yup that's why it also great to easily handle the dying script case: there's nothing specific to do :)

*/
public function __construct(\Memcached $memcached, $initialTtl = 300)
{
if (!static::isSupported()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to have an object instance of\Memcached and not having thememcached extension loaded?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It's technically possible that someone implement a\Memcached class.
If the class cover the original extension behaviors and is compatible, everything'll be fine.
If not, that's a weird case and should be addressed IMO.

@goetas
Copy link
Contributor

@jderusse Can you point me what guarantees thatdelete gets called for the memcache store as example? (https://github.com/jderusse/symfony/blob/48c998a097e8c71d90c5e8b508d21c8b866f2db1/src/Symfony/Component/Lock/Store/MemcachedStore.php#L113)

@goetas
Copy link
Contributor

I have tested theFlock and theRedis store.

With Flock, if the scripts terminates anomaly, the lock gets released by the kernel, but for redis this does not happen. I guess that memcache suffers from the same problem.

$predis =new \Predis\Client('tcp://localhost:6379');$store =new \Symfony\Component\Lock\Store\RedisStore($predis);$factory =new \Symfony\Component\Lock\Factory($store);$lock =$factory->createLock('foo');if ($lock->acquire()) {if (!rand(0,1))exit;// simulating failure$lock->release();}

After the failure occurs, the only way to unlock the lock, is to remove manually the entry from redis.

@jderusse
Copy link
MemberAuthor

@goetas None. But I can guarantee that Memcache will delete the key bat it own, when the TTL will expire.

@jocel1 it could be an issue to me. What if the connection closes while you think you are in lock phase (network issue, or mysql read timeout, ...)
Second issue. how do you know if you are the locker?

@goetas
Copy link
Contributor

@jderusse

None. But I can guarantee that Memcache will delete the key bat it own, when the TTL will expire

i knew this, the standard TTL that many key-value store have

@jderusse
Copy link
MemberAuthor

I suggest to read the (not yet merged) documentation about thishttps://github.com/symfony/symfony-docs/pull/7364/files

goetas reacted with thumbs up emoji

@goetas
Copy link
Contributor

@jderusse thanks for the explanation.

The documentation is clear and highlights the differences between different stores (advantages and disadvantages). At the beginning I was thinking that the all the stores implementations have to fulfill all the requirements (event when hardy possible)

@goetas
Copy link
Contributor

Mysql and postgres locks can be interesting to implement since they offer both advantages of local and remote locks :)

return false;
} catch (\Exception $e) {
$this->logger->warning('Failed to lock the "{resource}".', array('resource' => $this->key, 'exception' => $e));
throw new LockAcquiringException('', 0, $e);
Copy link
Member

Choose a reason for hiding this comment

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

@jderusse I suggest using a non-empty exception message for better DX

jderusse reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

indeed..

@jocel1
Copy link

jocel1 commentedMar 24, 2017•
edited
Loading

@jderusse

it could be an issue to me. What if the connection closes while you think you are in lock phase (network issue, or mysql read timeout, ...)
Second issue. how do you know if you are the locker?

If the connection closes on the locker side, it could indeed be problematic if you're not making subsequent requests on the db (in this case, the subsequent query would fail as well). But I would expect this to occur far less often than a TTL timeout or even an eviction prior to key expiry on memcached.
For the second issue, if you're the locker, you can call another GET_LOCK, the call will not be blocking. It's only blocking if you're calling the GET_LOCK in another session.

@nicolas-grekasnicolas-grekas modified the milestone:3.xMar 24, 2017
@patrick-mcdougle
Copy link
Contributor

patrick-mcdougle commentedApr 10, 2017•
edited
Loading

Can we eliminate the memcached store? Memcached is notorious for evicting keys that aren't even to their exptime because they are LRU than something else in the same slab class. By having it, developers will trust it more than they should. It's great as a cache, but really shouldn't be used as a persistent concurrent locking mechanism. Thoughts?

@jderusse
Copy link
MemberAuthor

Hi@patrick-mcdougle, can you please open an issue in order to follow this point?

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestApr 24, 2017
This PR was merged into the master branch.Discussion----------Add docs for the Lock componentSeesymfony/symfony#21093Commits-------3035fe9 Rename quorum into strategyd4326c0 Reduce usage of "you"d6a218c Rename quorum strategy17248e1 Remove codeblock6b6d865 Final review and some minor rewords/simplifications2498ccd Fix typod1d3b71 Fixed minor typos and syntax issues60bfe03 Fix typo6469016 Add the Lock Component
@fabpotfabpot mentioned this pull requestMay 1, 2017
nicolas-grekas added a commit that referenced this pull requestApr 30, 2018
…ry (corphi)This PR was merged into the 3.4 branch.Discussion----------[Lock][PhpDoc] There is no attempt to create the directory| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -When the lock component [was](#21093) [created](#22597), the `FlockStore` implementation was copied from the filesystem component. But the dependency to its origin was dropped, and as part of that, the attempt to create the parent directory for the locks. The PhpDoc wasn’t updated to reflect this change.Commits-------93a9bd3 PhpDoc: There is no attempt to create the directory
@jderussejderusse deleted the lock branchAugust 2, 2019 12:16
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

+7 more reviewers

@pierreduppierreduppierredup left review comments

@WironeWironeWirone left review comments

@goetasgoetasgoetas left review comments

@ro0NLro0NLro0NL left review comments

@linaorilinaorilinaori requested changes

@thewilkybarkidthewilkybarkidthewilkybarkid left review comments

@HeahDudeHeahDudeHeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[HttpFoundation] Session handlers should use a lock

18 participants

@jderusse@lyrixx@andersonamuller@javiereguiluz@fabpot@jjsaunier@goetas@jocel1@patrick-mcdougle@pierredup@nicolas-grekas@stof@Wirone@ro0NL@linaori@thewilkybarkid@HeahDude@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp