Catch exception from Connection::commit()

[morozov]

Connection::commit() will return void in DBAL 4. See doctrine/dbal#3480.

[derrabus]

DBAL 3's version of commit() might also throw exceptions already. Would it make sense to apply this change to the 2.11.x branch in that case?

[morozov]

DBAL 3's version of commit() might also throw exceptions already.

It can but those won't be the wrapper-level exceptions being caught in this patch. It could be a driver exception, a PDO exception, or anything else but I don't think that catching \Exception is a good idea since it won't necessarily identify a failed or even attempted commit.

I believe that since DBAL 3 doesn't provide an API for handling commit exceptions, the ORM shouldn't be using it.

[derrabus]

It can but those won't be the wrapper-level exceptions.

I was mainly thinking about ConnectionException which is a wrapper-level exception.

[morozov]

I was mainly thinking about ConnectionException which is a wrapper-level exception.

This is really out of scope. I'm trying to improve the ORM compatibility with DBAL 4, not address the API design issues of the earlier DBAL versions.

[derrabus]

Okay, but right now, the 3.0.x branch is compatible with DBAL 3 and we don't want to change that, do we? You would introduce a piece of logic that would catch exceptions that can be raised by DBAL 3 already.

At the moment, the ORM behaves differently for those exceptions than it does for a false return value. After your change, that difference would be gone. Is that what we want?

[morozov]

I definitely do not intend to change any existing behavior. What's the case with commit and ConnectionException thrown by commit() where this patch would change the behavior of the ORM? Would it be a change towards more consistent or less consistent error handling?

[derrabus]

Looking at the code of Connection::commit(), that exception is raised if the connection is in a state that would be really unexpected at this point:

        if ($this->transactionNestingLevel === 0) {
            throw ConnectionException::noActiveTransaction();
        }

        if ($this->isRollbackOnly) {
            throw ConnectionException::commitFailedRollbackOnly();
        }

Not catching that exception feels like the right thing to do. If DBAL 4 raises an exception for cases that resulted in a false return previously, maybe we should only catch that exception and let everything else bubble up?

[morozov]

These two exceptions being ConnectionException is wrong IMO. Purely from the common-sense perspective, a connection exception is a runtime exception that should be caught and possibly handled by retrying the operation. But these two are obviously logical exceptions and aren't meant to be caught.

I'm not really familiar with the logic of UnitOfWork, so I don't know how to proceed here. If you have any specific solution in mind, I can implement it.

[derrabus]

But these two are obviously logical exceptions and aren't meant to be caught.

Is there a way for a client that consumes a DBAL connection to distinguish between logic exceptions and runtime exceptions?

I'm not really familiar with the logic of UnitOfWork, so I don't know how to proceed here.

The code that we're about to change seems to assume that if a commit fails, it is because of a deadlock or similar situation. The exceptions raised by DBAL 4 should give us more insights on the reason for the failure. My idea would be to only catch exceptions related to locking and let everything else bubble up.

[derrabus]

Another thing: Later in that method, we have a generic catch block that attempts to clean up after a failed transaction:

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 442 to 452 in ad97969

	} catch (Throwable $e) {
	$this->em->close();
	if ($conn->isTransactionActive()) {
	$conn->rollBack();
	}
	$this->afterTransactionRolledBack();
	throw $e;
	}

Note that the return value of the rollBack() call is ignored. I think this is on purpose: We attempt a rollback and if that fails, we simply move on because the initial exception is more valuable than a potential rollback failure. If rollBack() also throws instead of returning false, I think we need to catch&ignore possible exceptions here.

[morozov]

I believe it's better to wait until we can run the test suite. Otherwise, we're shooting in the dark.

The major blocker for running the suite is compile-time errors caused by the hard-coded mocks (#8886 (comment)).