Fix incorrect handling of transactions using deferred constraints #3424

grongor · 2019-01-04T15:22:11Z

Q	A
Type	bug
BC Break	no
Fixed issues	#3423

Summary

Rollback called when outside of a transaction when using deferred constraints (PostgreSQL).

grongor · 2019-01-04T16:01:56Z

Not sure why the continuousphp build failed - I think it's not related to my changes.

Travis's MySQL died for some reason, here is the build of this branch against my fork: https://travis-ci.org/grongor/dbal/builds/475348248

Ocramius · 2019-01-04T16:11:45Z

lib/Doctrine/DBAL/Connection.php

        } catch (Exception $e) {
            $this->rollBack();
            throw $e;
        } catch (Throwable $e) {
            $this->rollBack();
            throw $e;
        }
+
+        $this->commit();


Shouldn't a crash on commit() lead to a rollback as well?

That is precisely the issue here - when you call commit() and it fails, then the transaction is no more :) There is nothing to rollback ... so this PR is all about not calling the rollback if the failure occurs in the commit()

then the transaction is no more

Ok, this is my misconception then. Are we sure that this holds consistently for all platforms that we support?

AFAIK all other platforms throw the exceptions before commit() (they do not support constraint deferring), so we will always call rollback in those cases. But that might not be true - I'm not familiar enough with all of them to be perfectly honest.

Anyway, shouldn't the existing tests cover that?

shouldn't the existing tests cover that?

If they run on all platforms, yes.

I'll have to defer that decision to @deeky666 and @morozov

What about a case where the Exception happens during an open transaction? Shouldn't this improvement be marked as a BC break, when there are no rollbacks anymore?

@SenseException I'm sorry, I don't understand. I don't think that I changed the behavior for "common" transactions in any way; please provide an example code if you think so - thanks.

You fixed your issue "Rollback called when outside of a transaction" in this method by moving the commit() call out of the try-catch. Before your change an exception thrown in commit() would call a rollback in the catch, but now this isn't the case anymore. What if commit() throws an exception inside a transaction and a rollback is needed?

Thanks, yeah, I understand now.

What if commit() throws an exception inside a transaction and a rollback is needed?

Then that case wouldn't be handled. The thing is that I was not able to come up with an example where COMMIT would throw an exception and also where would need to call ROLLBACK afterwards. I asked all devs in my company and no one was able to think of a situation like that. I also asked one guy who knows basically everything around MySQL and he was not able to think of such example (even tried poking master/slave and cluster setups).

But I guess unless someone checks all source codes/documentations of all supported databases we can't be 100 % sure. All I can do is to deploy those changes to our production systems (Postgres and MySQL) and report back if we experienced anything unusual...

Or can you think of a better solution to this problem?

Ocramius · 2019-01-04T16:13:04Z

lib/Doctrine/DBAL/Connection.php

+        $this->commitDone();
+    }
+
+    private function commitDone()


: void

Also: naming suggests that committing finished, yet the code below begins a new transaction now?

Documentation block also needed - or better/more explicative naming.

Oh yeah, I wasn't sure about that part at all :D Do you have any suggestions for the name? It basically just resets the internal state of the object, after the commit.

If it is about resetting state, then the call to ->beginTransaction() should be removed and replaced with direct state modification.

I don't have a suggestion for the name, because I don't understand this part of the diff myself either.

Purpose of this method is to set the Connection to the correct state. Until now the code didn't expect the commit() to ever throw any exception. Now that it is possible we need to manage the state so that if someone catches the exception, then they can still use the connection afterwards. Without doing that the connection would be stuck with transactionNestingLevel set to 1 even though the transaction is long gone. The ->beginTransaction() is there because of the autoCommit thingie - without it the Connection object would again be in an unexpected state.

Right, and that isn't clear by reading the method: can we somehow improve that?

Consider comments to be the last resort: having a clear method name would be a good improvement at first.

@Ocramius added the patch. Does updateTransactionStateAfterCommit() make more sense?

tests/Doctrine/Tests/DBAL/Functional/Ticket/GH3423Test.php

SenseException · 2019-01-08T22:50:40Z

lib/Doctrine/DBAL/Connection.php

@@ -1172,15 +1172,17 @@ public function transactional(Closure $func)
        $this->beginTransaction();
        try {
            $res = $func($this);
-            $this->commit();
-            return $res;
        } catch (Exception $e) {


Can we catch Exception and Throwable in one catch here? Or Throwable only?

I didn't want to do more changes than was really necessary in this PR, but yeah, I think catching Throwable only is a sensible and possible. Should I add the patch to this PR?

SenseException · 2019-01-08T22:57:03Z

tests/Doctrine/Tests/DBAL/Functional/Ticket/GH3423Test.php

+     */
+    public function testCommitWithDeferredConstraintAndTransactionNesting() : void
+    {
+        $this->expectException(DBALException::class);


To prevent possible future BC breaks, I suggest to put expectException() before the line, that throws the actual exception.

Yeah I usually do it like that but all tests in DBAL seemed to start with it, so I just went with it ... I'll update the code shortly

SenseException · 2019-01-08T23:13:58Z

lib/Doctrine/DBAL/Connection.php

        } catch (Exception $e) {
            $this->rollBack();
            throw $e;
        } catch (Throwable $e) {
            $this->rollBack();
            throw $e;
        }
+
+        $this->commit();


What about a case where the Exception happens during an open transaction? Shouldn't this improvement be marked as a BC break, when there are no rollbacks anymore?

morozov

We need a test which covers the change on all platforms, not only on those which support deferred constraints.

morozov · 2019-01-09T00:27:50Z

tests/Doctrine/Tests/DBAL/Functional/Ticket/GH3423Test.php

+    {
+        parent::setUp();
+
+        if (! in_array($this->connection->getDatabasePlatform()->getName(), ['postgresql', 'oracle'], true)) {


It's a bit scary to make a change to a core function and then test it only on two platforms. This specific test will require the platform to support deferrable constraints, but can a commit fail for a different generic reason which is applicable to all platforms?

@morozov Yeah, I understand the fear, trust me :D I don't see another way to do it - how would I test something that the other platforms don't support? I'm not aware of a way to do that ... I think it must be sufficient that these changes don't break the existing tests as those tests should cover all the core/common functionality. And just to be extra sure I adjusted the ConnectionTest to let the database throw the errors, you can check it out here #3425 .

And as far as I know the commit won't fail under a normal operation (other then connection lost, etc.). And even if it would fail, I think it would make sense that the transaction would be closed afterwards (what else the DB can expect, if we tried to commit and failed?). But yeah, I would love to know that for sure and add some tests for this :D But I can't think of any situation that would trigger error on commit (other then the deferred transactions).

Here is a build on top of that improved ConnectionTest https://travis-ci.org/grongor/dbal/builds/479358748 - passing

how would I test something that the other platforms don't support? I'm not aware of a way to do that ...

Except for not being able to create a deferred constraint, how is the new test expected to behave on a platform that doesn't support deferred constraint? I.e. instead of skipping the test entirely, can we just create the deferred constraint if the platform supports it, and otherwise create a regular one?

I think it must be sufficient that these changes don't break the existing tests as those tests should cover all the core/common functionality.

Could you identify these tests?

grongor · 2019-01-14T11:16:30Z

@morozov I think that we do have a test that covers it on all platforms: the functional ConnectionTest. I don't see a better way to test this. The new test GH3423Test just covers that extra portion that only postgres and oracle support.

danydev · 2020-06-06T09:01:27Z

@morozov @grongor we recently hit this bug as well. Since it looks like quite a bit of work has been done, do you think the fix is gonna land in doctrine soon? Do you need help in some specific task to get it done?.

morozov · 2020-06-06T18:07:55Z

tests/Doctrine/Tests/DBAL/Functional/Ticket/GH3423Test.php

+/**
+ * @see https://github.com/doctrine/dbal/issues/3423
+ */
+class GH3423Test extends DbalFunctionalTestCase


Please avoid using issue numbers in tests. It should be named after the feature/behavior being tested.

morozov · 2020-06-06T18:13:30Z

[…] we recently hit this bug as well. Since it looks like quite a bit of work has been done, do you think the fix is gonna land in doctrine soon?

@danydev, if fixing this bug doesn't require breaking changes, this PR may be retargeted against 2.10.x. This way, it could be released soon after it's resolved.

At the moment, there's no enough confidence that changing the code at the abstraction level and testing it only with certain platforms is sufficient.

Do you need help in some specific task to get it done?

You can help by participating in the conversation https://github.com/doctrine/dbal/pull/3424/files#r436288581.

grongor · 2021-03-12T11:59:56Z

So ... I tried rebasing it today (4.x), and just running the tests against the current implementation. Here is the build: https://github.com/grongor/dbal/actions/runs/646044144

Postgres is broken: the constraint violation is completely throw away and replaced with "There is no active transaction".
Oracle passes the test, as it includes the information about the constraint violation in the exception throw when rolling back the transaction. You can see it here https://github.com/grongor/dbal/runs/2094504708?check_suite_focus=true#step:6:70 (where I yet didn't handle the OCI warning). I personally think that the rollback here isn't correct either, but I'm not familiar with Oracle and I don't intend to be :D

So ... do you want to continue this somehow, or should we close? I checked the tests around the Connection class and it seems they are a bit too synthetic. We could add a test that would check the constraint violation (without deferring), and if the new implementation won't break it, I think it's safe to say it will fix this issue without causing another. Would that be enough for this to get merged? If so, I'll update the PR.

ps: Sorry for the "late response", it cost me a lot of time and I got tired at some point. We also stopped using the deferred constraints so it wasn't a priority for me anymore...

morozov · 2021-03-12T16:38:59Z

I personally think that the rollback here isn't correct either, but I'm not familiar with Oracle and I don't intend to be :D

That's the problem. The code being modified works for all platforms.

So ... do you want to continue this somehow, or should we close?

What do you want? If you want to close it, feel free to. If you want to proceed, you'll need to gather more information. Likely, not all platforms will behave the same, and the code change will become more complex. E.g. here (just as a reference) it's stated that a rollback is needed. And here is an example of using tx.getStatus() to observe the transaction status to make the call. What could help:

Research the documentation on all supported platforms and see if a rollback is needed after a failed commit.
Find a way to observe the status of a transaction programmatically and write tests.
Research other projects like JDBC and see how it's done there.

I checked the tests around the Connection class and it seems they are a bit too synthetic.

What do you mean?

grongor · 2021-03-12T16:59:09Z

That's the problem. The code being modified works for all platforms.

No, it does not. See the link I provided. It doesn't work for the deferred constraints.

E.g. here (just as a reference) it's stated that a rollback is needed

Nope. To quote exactly: either explicitly with a COMMIT or ROLLBACK statement. And my proposed change does exactly that. Your current implementation does both, which is what causes the unexpected errors (again, please check the links to the build I provided in the comment above).

What do you mean?

There isn't a single test that would check how the transaction behaves when there is an error, like for example violation of a constraint. By an error, I mean something that is triggered by the database, not something we throw explicitly in the test.

What do you want?

Nothing really. I just didn't want to be that guy that just closes the issue without giving it at least one last try. I've given this issue (and the one in ORM package) a lot of time already, and I only met with resistance, which to be honest I understand and think is warranted. I just expected someone to meet me half-way. I'm not gonna spend another 20 hours just to learn that "we aren't quite confident yet" - I hope you can understand that.

Maybe there is another way to approach this. I wanted to remove the rollback call because in the case of deferred statements it is probably redundant (and causes exception), but maybe we could try to put another try-catch around the rollback statement. If there is an exception, we can ignore it and re-throw the "original" exception that caused the rollback in the first place. That way we wouldn't have to change how the commit/rollback is handled, and we would also solve the issue with deferred constraints, where the actual error is hidden away by the "there is no transaction" error caused by the rollback.

danydev · 2021-03-12T17:20:22Z

It's kind of hard to guarantee that a rollback is not needed after a failed commit in all vendors. For example sqlite may return a SQLITE_BUSY exception, and after that you still need to rollback.

I think your second proposal may be the way to sort this out. Maybe It's not the cleanest, but it's the safer.

grongor · 2021-03-12T17:40:40Z

Yeah I agree. To be honest I am quite surprised I didn't come up with it sooner lol, now it seems like an obvious way to do it.

simPod · 2021-08-18T07:11:09Z

@morozov what branch should this target?

morozov · 2021-08-18T15:07:19Z

I've retargeted to 3.1.x.

Co-authored-by: Simon Podlipsky <simon@podlipsky.net>

grongor mentioned this pull request Jan 4, 2019

Rollback called when outside of a transaction when using deferred constraints #3423

Open

grongor mentioned this pull request Jan 4, 2019

Fix incorrect handling of transactions using deferred constraints doctrine/orm#7556

Open

Ocramius suggested changes Jan 4, 2019

View reviewed changes

Ocramius added Bug PostgreSQL Error Handling Transactions labels Jan 4, 2019

Ocramius added this to the 2.9.3 milestone Jan 4, 2019

Ocramius reviewed Jan 4, 2019

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/Ticket/GH3423Test.php Outdated Show resolved Hide resolved

grongor mentioned this pull request Jan 7, 2019

Improve ConnectionTest by triggering real exceptions #3425

Merged

SenseException requested changes Jan 8, 2019

View reviewed changes

morozov requested changes Jan 9, 2019

View reviewed changes

morozov removed this from the 2.9.3 milestone May 22, 2019

morozov reviewed Jun 6, 2020

View reviewed changes

grongor mentioned this pull request Jan 11, 2021

Rollback called when outside of a transaction when using deferred constraints doctrine/orm#7555

Open

Base automatically changed from master to 4.0.x January 22, 2021 07:44

simPod force-pushed the fix-deferred-constraint--pr branch from dabc8ac to 7db0d8b Compare August 17, 2021 12:55

grongor changed the base branch from 4.0.x to 2.13.x August 17, 2021 12:57

simPod force-pushed the fix-deferred-constraint--pr branch from 7db0d8b to 39c25c5 Compare August 17, 2021 12:59

morozov changed the base branch from 2.13.x to 3.1.x August 18, 2021 15:06

simPod force-pushed the fix-deferred-constraint--pr branch 14 times, most recently from 109efc6 to e805a0f Compare August 20, 2021 08:17

Fix incorrect handling of transactions using deferred constraints

8b8f169

Co-authored-by: Simon Podlipsky <simon@podlipsky.net>

simPod force-pushed the fix-deferred-constraint--pr branch from e805a0f to 8b8f169 Compare October 4, 2021 08:21

simPod mentioned this pull request Oct 4, 2021

Fix incorrect handling of transactions using deferred constraints #4846

Closed

morozov closed this Oct 26, 2021

github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022

Fix incorrect handling of transactions using deferred constraints #3424

Fix incorrect handling of transactions using deferred constraints #3424

Conversation

grongor commented Jan 4, 2019

Summary

grongor commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grongor Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grongor commented Jan 14, 2019

danydev commented Jun 6, 2020

Choose a reason for hiding this comment

morozov commented Jun 6, 2020 • edited Loading

grongor commented Mar 12, 2021

morozov commented Mar 12, 2021 • edited Loading

grongor commented Mar 12, 2021

danydev commented Mar 12, 2021 • edited Loading

grongor commented Mar 12, 2021

simPod commented Aug 18, 2021

morozov commented Aug 18, 2021

grongor Jan 14, 2019 •

edited

Loading

morozov commented Jun 6, 2020 •

edited

Loading

morozov commented Mar 12, 2021 •

edited

Loading

danydev commented Mar 12, 2021 •

edited

Loading