Problem/Motivation

I have a custom module that needs to send article information (title, url, etc) to an external REST API after every update. It uses post/root transaction callback to do it (after DB transaction) and worked fine with 10.1.x. After updating to latest 10.2.x tests are failing with following error:

Drupal\Core\Database\TransactionNameNonUniqueException: A transaction named drupal_transaction is already in use. Active stack: 66434febcfa244.10114297\drupal_transaction

With assertions enabled the error message is:

PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught AssertionError: Transaction $stack was not empty. array (
  '66434266067936.22564777' =>
  \Drupal\Core\Database\Transaction\StackItem::__set_state(array(
     'name' => 'drupal_transaction',
     'type' =>
    \Drupal\Core\Database\Transaction\StackItemType::Root,
  )),
) in drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:100
Stack trace:
#0 drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php(100): assert()
#1 [internal function]: Drupal\Core\Database\Transaction\TransactionManagerBase->__destruct()
#2 {main}
  thrown in drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php on line 100
Fatal error: Uncaught AssertionError: Transaction $stack was not empty. array (
  '66434266067936.22564777' =>
  \Drupal\Core\Database\Transaction\StackItem::__set_state(array(
     'name' => 'drupal_transaction',
     'type' =>
    \Drupal\Core\Database\Transaction\StackItemType::Root,
  )),
) in drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:100
Stack trace:
#0 drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php(100): assert()
#1 [internal function]: Drupal\Core\Database\Transaction\TransactionManagerBase->__destruct()
#2 {main}
  thrown in drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php on line 100

drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97

According to git bisect 27bb1ff6 is the first bad commit.

If I add \Drupal::urlGenerator()->generate('<front>'); to test ::setUp() the error disappears.

Steps to reproduce

  1. Modify entity_crud_hook_test.module:
    diff --git a/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module b/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module
index 3d6bc4c9d6..3346981cef 100644
--- a/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module
+++ b/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module
@@ -94,8 +94,16 @@ function entity_crud_hook_test_file_presave() {
 /**
  * Implements hook_ENTITY_TYPE_presave() for node entities.
  */
-function entity_crud_hook_test_node_presave() {
-  $GLOBALS['entity_crud_hook_test'][] = (__FUNCTION__ . ' called');
+function entity_crud_hook_test_node_presave($node) {
+  $GLOBALS['entity_crud_hook_test'][] = (__FUNCTION__ . ' called');
+  $db = \Drupal::database();
+  if ($db->inTransaction()) {
+    $db->addRootTransactionEndCallback(function ($success) use ($node) {
+      if ($success) {
+        $node->toUrl()->toString();
+      }
+    });
+  }
 }

 /**
  2. Run test: 
    SIMPLETEST_DB=sqlite://tmp/core.sqlite vendor/bin/phpunit -c core core/tests/Drupal/KernelTests/Core/Entity/EntityCrudHookTest.php --filter=testNodeHooks
  3. See error: 
    1) Drupal\KernelTests\Core\Entity\EntityCrudHookTest::testNodeHooks
Drupal\Core\Database\TransactionNameNonUniqueException: A transaction named drupal_transaction is already in use. Active stack: 664358001e5317.21795200\drupal_transaction

drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:234
drupal/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php:603
drupal/core/lib/Drupal/Core/Routing/MatcherDumper.php:119
drupal/core/lib/Drupal/Core/ProxyClass/Routing/MatcherDumper.php:84
drupal/core/lib/Drupal/Core/Routing/RouteBuilder.php:194
drupal/core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php:83
drupal/core/tests/Drupal/KernelTests/RouteProvider.php:31
drupal/core/tests/Drupal/KernelTests/RouteProvider.php:48
drupal/core/lib/Drupal/Core/Routing/UrlGenerator.php:443
drupal/core/lib/Drupal/Core/Routing/UrlGenerator.php:276
drupal/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php:108
drupal/core/lib/Drupal/Core/Url.php:765
drupal/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module:103
drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:447
drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:433
drupal/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:291
drupal/core/lib/Drupal/Core/Database/Transaction.php:93
drupal/core/lib/Drupal/Core/Entity/EntityBase.php:354
drupal/core/tests/Drupal/KernelTests/Core/Entity/EntityCrudHookTest.php:326
drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Proposed resolution

TBD

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3447097

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

olli created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Maybe related to #3440848: Ensure post transaction callbacks are only at the end of the root Drupal transaction?

olli’s picture

olli commented

In general this happens if you add a post transaction callback with anything that starts a transaction. In this particular case it might be the route provider used in kernel tests.

To reproduce this outside tests, add a post transaction callback that saves a node. For example:

function example_node_presave(\Drupal\node\NodeInterface $node) {
  $db = \Drupal::database();
  if ($db->inTransaction()) {
    $db->transactionManager()->addPostTransactionCallback(function ($success) use ($node) {
      if ($success) {
        if ($node->label() === '#3447097') {
          $node->set('title', 'issue 3447097');
          $node->save();
        }
      }
    });
  }
}

Add or edit node, set title to #3447097 and save.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Yep I've just run into this bug... it's because we fire the callbacks before we've emptied the stack and it is definitely a bug.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

This is fixed on 11.2.0 by #3398767: Allow returning explicitly to the prior nesting level in transactions (aka allow explicit COMMIT in Transaction objects) = this issue could add test coverage.

alexpott opened merge request !13198

alexpott opened merge request !13199

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

A bit counterintuitive that a ‘post-transaction callback’ starts a transaction of its own but yeah, life is difficult.

Can we have the same test also doubled so to use commitOrRelease instead of unset? So we can make the ones using unset to be deprecation tests when converting all transaction to explicit commit.

EDIT - such duplicate test methods could live in TransactionTest instead of the base class.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@mondrake done that and updating to use ->commitOrRelease() proves we have a problem. The callbacks are only triggered on transaction destruction - not when ->commitOrRelease() processes the root commit.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3495728: [meta] Drop implicit autocommit on Transaction destruction, +#3406985: Convert all transactions in core to use explicit ::commitOrRelease()

Actually, that is on purpose, as it's the current behavior with commit-on-destruct. Even if extremely edge, in the current scenario you may have db operations between ::commitOrRelease() and ::destruct(), and the callback rely on those.

I was planning to do that as a follow-up of #3406985: Convert all transactions in core to use explicit ::commitOrRelease(), see plan in #3495728: [meta] Drop implicit autocommit on Transaction destruction, point 4 in the IS; but of course, it can be put forward if we decide so.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Second thinking. Isn't there a risk of endless looping by allowing a post-transaction callback to start another transaction of its own? That second transaction may register post-transaction callbacks again, and so forth. Don't think this was considered in the original intent for introducing post-transaction callbacks.

I think this needs some thinking/agreement on what should be the behavior we have to support.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.