Identify retryable transaction errors and cause them to raise specific exception types

[Tobion]

So this PR now only contains the explicit exceptions so they can be catched RDMBS agnostic.

Below description can be provided externally.

It is best practice to implement retry logic for transactions that are aborted because of deadlocks or timeouts. This makes such method available inside the DBAL and also adds detection for errors where retrying makes sense in the different database drivers.

Deadlocks and timeouts are caused by lock contention and you often can design your application to reduce the likeliness that such an error occurs. But it's impossible to guarantee that such error conditions will never occur. This is why implementing retrying logic for such errors is actually a must when you have to ensure the application does not fail in edge cases or high load.
Some references where something similar has already been discussed and implemented:

• SQL Server
• Java Spring with Hibernate
• Google App Engine

I chose the name retryable because it is consistent with transactional. I think the implementation is quite straight forward and fits very well with the DBAL design.

In our case we had seldomly errors like

• Doctrine\\DBAL\\Exception\\DriverException: An exception occurred while executing 'UPDATE product SET modified = ? WHERE id = ?' with params [\"2014-10-15 16:28:55\", \"460315800000\"]:\n\nSQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction
• Doctrine\\DBAL\\Exception\\DriverException: An exception occurred while executing 'INSERT INTO ... VALUES (...)' with params [\"...\", \"...\"]:\n\nSQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction

As you can see even the exception message suggests to retry the transaction. This is now easily possible with

$retryWrapper = new RetryWrapper(function () use ($con) {
    return $con->update('tablename', array('field' => 'data'), array('id' => 'myid'));
});

$affectedRows = $retryWrapper();

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1035

We use Jira to track the state of pull requests and the versions they got
included in.

[Ocramius]

I don't think this should be an additional responsibility of the Connection object: can't we just use a new class for it?

class RetriableCallable
{
    public function __construct(callable $retriable) { ... }
    public function __invoke() { ... }
}

[stof]

wouldn't it be better to make it a marker interface instead ?

[Tobion]

I don't think so because catching an interface that is not an exception (since there is not exception interface in php) makes hardly sense since you actually also don't have access to getMessage() or even getSqlState() from DriverException. Furthermore Doctrine does not use marker interfaces for exceptions (at least I didn't find any).

[Ocramius]

Furthermore Doctrine does not use marker interfaces for exceptions

That's actually something that we should start doing, heh...

[deeky666]

I would even go further and say we should have something like TransactionException extends ServerException to have a base class for all transaction exceptions. Then DeadlockException and LockWaitTimeoutException should implement the marker interface RetryableException. Another idea would be to name that RetryableException ConcurrencyException (because that's what it actually is) although I'm not sure whether every concurrency exception can be retried. What do you think?

[Tobion]

Everything is a transaction in database terms. So what's the point?

[stof]

@Tobion Catching on an interface still gives you access to getMessage(), given that the fact that it is a catch block tells you that the object is also an exception, not only an instance of this interface (the fact that IDEs don't support autocompletion properly in such case is not an issue in the usage of an exception for that, but in the inference engine of the IDE)

[Tobion]

true, I changed it

[stof]

you should allow any callables (we have Closure typehints in several places in Doctrine because of a bad decision years ago and the fact that breaking BC to change the typehint is not worth a major version by itself, so it will wait until a real need for a major version)

[ChristianRiesen]

+1
I can't see where an extra class would make sense, since transactional is in there too. The scaffolding needed for an extra class would negate the sheer convinience of having this inside teh connection.

I see it following patterns of current Doctrine standards. If different patterns should be used, thats what the next version is for IMHO.

Looking forward to using this!

[Ocramius]

@ChristianRiesen if we start introducing new APIs now then the migration path for 3.x will just be harder. Let's not break SRP for the sake of usability if it's just going to damage us.

[ChristianRiesen]

I see your argument @Ocramius but by that definition any pull request that isn't a bug fix or essentially, a 3.0 enhancement, is mute. That would hurt the doctrine project more imho.
The implementation here is clean and portable. A custom implementation outside of doctrine is in any case a klutz.
If you have a better way of doing it, just give an idea how the signature would look like and an example how it would be used then. I think that would bring this a lot further a lot quicker. I for one can't wait to use this, whatever form it takes.

[Tobion]

Thinking about it, I feel @Ocramius is right that it does not necessarily belong on the Connection. transactional operates on a connection directly with beginTransaction etc. But retry logic does not really need the connection and could for example also use a repository method which is retried. Having an extra class would also remove the obvious inconsistency about callable vs Closure between transactional and retryable that stof wants to be changed.

So I'm willing to change it to an invokable class. How about Doctrine\DBAL\RetryCallable as name?
And does Doctrine agree with the general implementation and is willing to accept this PR? I could also write a blog post about this to introduce this feature.

[dbu]

what about a decorator class around the Connection?

[Ocramius]

what about a decorator class around the Connection?

Why is this sort of API in first place a responsibility of the Connection object? :-)

[deeky666]

Maybe we should consider integrating this feature into #634 instead? Not sure if there is a better place for this functionality...

[Tobion]

I updated this PR. The retry logic is now in a separate class which is much nicer and flexible. I also added unit tests.

@deeky666 #634 is unrelated to this PR.

[Tobion]

The class name is RetryWrapper. I think RetryDecorator would be more correct. Wrapper is usually used in context of adapter pattern. But "wrapper" seems to be used in doctrine, like wrapperClass. So what do you guys prefer?

[stof]

@Tobion RetryDecorator would not be more correct, given that it is not a decorator. If you want a more correct name, RetryingCallable seems much more meaningful

[Tobion]

@stof Why should it not be a decorator? It decorates a callable as a callable that adds functionality (retry logic). This is exactly the definition of the decorator pattern.

[stof]

this needs to extends Exception, otherwise weird things will happen

[Tobion]

Thinking about it, the RetryWrapper is actually totally generic and the use-case would not be limited to database operations. For example you can also use it for instable http operations. So actually it might be worth extracting it into a separate library, similar to https://github.com/nfedyashev/retryable in ruby.
DBAL would then need to require this library to configure via subclass for example that only RetryableExceptions are actually retried. Would doctrine accept to require or suggest in composer.json such a generic retryable library?

[Ocramius]

Would doctrine accept to require or suggest in composer.json such a generic retryable library?

I doubt that we would include an external dependency for such a small library right now

[Tobion]

It could be an optional dependency which would only be required when you need the retry logic for your db operations.

[stof]

It could be an optional dependency which would only be required when you need the retry logic for your db operations.

It cannot be optional if doctrine extensions are implementing the marker interface of the library

[Tobion]

The marker interface would not need to be part of the library. The library would catch all exceptions by default and allow to configure to retry only on specific ones (as in the ruby library), like RetryableException in DBAL.

[stof]

for what it is worth, https://github.com/igorw/retry just misses the possibility to filter which exceptions should be retried

[Tobion]

Hm I didn't know about this one. But it also has no delay. Maybe @igorw and me should put our efforts together.

[stof]

@Tobion the delay is still in a PR because there is a discussion about the way to implement it

[Tobion]

@Ocramius @stof I removed the retry logic from the PR as this can be provided by another package (https://github.com/Tobion/retry). So this PR now only contains the explicit exceptions so they can be catched RDMBS agnostic. Please consider merging.

[Tobion]

@deeky666 @Ocramius ping

[Tobion]

@deeky666 ping

[Tobion]

@deeky666 any schedule for this?

[ChristianRiesen]

I'd like to know the same. This has been open forever.

[Tobion]

ping

[deeky666]

@Tobion @ChristianRiesen I'm sorry for the delay, haven't been very active during the last year. Merging this now. Will land in 2.6. Still there are two things missing which should be added before releasing. First, the implementation for Oracle is missing and second I think it would be good to have at least a little chapter about retryable exceptions in the documentation about transactions. PRs welcome.
Otherwise thanks for the great effort! Much appreciated :)

[deeky666]

Created issues for both tasks:

#2289
#2290

[mishriky]

Any chance this change would get tagged soon?

[ChristianRiesen]

+1