should be final

Why not, but why ?

We keep having this conversation again and again :)
We don't do final by default in Symfony, it would reduce the value of the code base with no real benefit for maintenance.

On the contrary, it increases the value of the code base, because the developer can clearly see which are extension points and which are not. It will also force the developer to use composition and service decorators, because they can't get around with ugly hacks such as changing the service class.

It will actually make maintenance a lot easier because it's no longer an extension point 🤔

We don't do final by default in Symfony

Maybe we should ;)

We don't do final by default in Symfony

Wait, but it is not by default, it's by@iltar's request. :) I don't see valid use case to inheritMysqlStore as well. What's the point to support it? 🤔

Do we still keep inheritDoc comments?

Yes

Here you open a new connection for each lock (is it necessary?). It could trigger a max connections limit. Moreover you don't close theme.

The connection is closed automatically when thePDO instance is destructed. That's done by removing thePDOStatement from the key state.

I chose to open a new connection for each lock to be sure that we don't get the same lock multiple times on the same connection.

In MySQL 5.7.5,GET_LOCK() was reimplemented using the metadata locking (MDL) subsystem and its capabilities were extended. Multiple simultaneous locks can be acquired and GET_LOCK() does not release any existing locks.It is even possible for a given session to acquire multiple locks for the same name. Other sessions cannot acquire a lock with that name until the acquiring session releases all its locks for the name.

But if we allow the injection of a connection, we need to keep this state in-memory in the MysqlStore instance.

Indeed by sharing PDO you have to to avoid concurrency inside the same process. This side effect should be warned in the documentation IMO

I've found this workaround to avoid multiple connections:

SELECT IF(IS_USED_LOCK(:key) ISNULL, GET_LOCK(:key, :timeout),0)

Commit is coming.

be sure that we don't get the same lock multiple times on the same connection.

Is this really what we want? Looks opinionated to me. flock() doesn't blocks when acquiring twice a lock on a file, I would expect the same here, so that I have more choices in the end (my responsibility to create several connections if I need intra process locking)

@nicolas-grekas there is a test for that (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Tests/Store/AbstractStoreTest.php#L80). Tools like amphp (ReactPHP) needs locks inside the same process.

But same key (remember, a key contains the resource locked + the lock state) can be reused and locked several times inside the same process.

Ok indeed

In others stores blocking mode is waiting forever. Such option is not needed IMO

The "wait timeout" feature was not available in flock and semaphore, isn't it the reason why it was not implemented? It is a useful alternative toRetryTillSaveStore when the lock needs to be acquired in a limited time with a clean error handling.

It wasn't implemeted in the store because I didn't see end to end use case, and such feature can be handled by a decorator and bring the feature to every store.

hmm if someone uses the passed$connection elsewhere (maybe in a long running cli process) then this could lead to surprises as it can be closed earlier than expected?

This SQL statement can only increase thewait_timeout (takes the max between the current value and the minimum expected value).

If the connection is closed intentionally, then the lock is automatically released. It is a limitation/feature of this locking mecanism.

Throwing that exception in blocking mode is weird IMO, the purpose of the the blocking is to wait until the lock can be acquired.

Use case: With ReactPHP and a Session Storage that used this MySQL store and 2 concurrent HTTP request (think ajax). The second HTTP request shouldn't fail just because it performs a "session_start" which trigger the lock

By default, the wait timeout is infinite (-1). This exception will be thrown in a specific scenario that is not

I've updated to use this "wait timeout" feature in non-blocking mode and throw the sameLockConflictedException if the lock is token by an other or the by the same connection.

now connection is injected in this class, do we still need to store the statement in the key?

No, I've updated the code to prepare the statement in thedelete method.

good catch

This SQL statement can only increase thewait_timeout (takes the max between the current value and the minimum expected value).

If the connection is closed intentionally, then the lock is automatically released. It is a limitation/feature of this locking mecanism.

By default, the wait timeout is infinite (-1). This exception will be thrown in a specific scenario that is not

I've updated to use this "wait timeout" feature in non-blocking mode and throw the sameLockConflictedException if the lock is token by an other or the by the same connection.

No, I've updated the code to prepare the statement in thedelete method.

@jderusse This query insure that: 1. the connection is still alive & 2. the lock is still assigned to the current connection.

To be acquired, and not to be released right?

It will be released after the timeout so that sounds right to me. You could also say "The time in seconds the lock is acquired for".

Regardingthe documentaiton the parameter timeout, define the maximum amount of time allowed to acquire the lock. Once you get it, you can keep it as long as you want.

T0: ask for acquiring a new lock (wait for other process to release the lock for instance)
T1: lock is acquired
T2: lock is released

Here we are talking about the time between T1 and T0.

Null, process killed by admin, or machine out of memory