Sessions are invalidated on subsequent page request when written while transaction is open

No regressions in the existing test suite. I think the correct answer to this is 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed but that change won't come until at least Drupal 12.

Some discussion on Slack as well.

OMG. This feels wrong. commitAll() was introduced in 🐛 Transaction autocommit during shutdown relies on unreliable object destruction order Active to fix during shutdown an edge case of xdebug keeping alive Transaction objects that should have been naturally destructed. Using it in normal operations means you cannot rely on transaction state after it being called.

I think the question here is not even related to implicit vs explicit commit, but rather why should a transaction be active at this point.

Thanks for chiming in.

I think the question here is not even related to implicit vs explicit commit, but rather why should a transaction be active at this point.

Perhaps the question isn't directly implicit vs. explicit, but since Drupal currently supports implicit transaction commits/completion, then why wouldn't there be a transaction active?

I dropped a breakpoint on TransactionManagerBase::push() and found that the open transaction was started in SqlContentEntityStorage::save(), where it is not explicitly destroyed. So core is opening the transaction. What's more, the object isn't returned to the caller so there's no way to explicitly commit. If you follow the instructions on the transaction docs, then yes, you could start a transaction, then ::save() would add a savepoint, and then the transaction could be explicitly committed. But that would mean a pretty significant shift for developers to always have to wrap their entity saves in transactions in order to avoid the condition that prompted this issue.

That also explains why I only experience this bug after a user is created, because of the entity save operation.

SqlContentEntityStorage::save() should be automatically committing the transaction when the transaction goes out of scope.. so if that's not happening, I guess something else already started a transaction?

Re: #8 - At first I was like, duh, yes it would be destroyed when going out of scope, I was barking up the wrong tree.

Except then, I tried to go in and reconcile each transaction "push" with a destruction. Each push was from ::save(), however I had two that were not cleaned up. I even added an explicit unset($transaction) before the return and same result - there were two objects that were only destroyed later during shutdown.

I think this is at least in part due to the fact I am doing an entity save during an entity insert hook - e.g., I create a profile entity for the newly-created user. I suspect this is related somehow. There's no other reference to the Transaction object returned from ::push(), so I'm kinda mystified. PHP must not be allowing the reference to be destroyed, even when I explicitly unset() it in ::save(). Does this ring a bell to anyone?

I am curious if this is an edge case with TransactionManagerBase. In my case, two transactions that are not destroyed seem to fall under this docblock in ::unpile():

    // If the $id does not correspond to the one in the stack for that $name,
    // we are facing an orphaned Transaction object (for example in case of a
    // DDL statement breaking an active transaction). That should be listed in
    // $voidedItems, so we can remove it from there.
    if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
      unset($this->voidedItems[$id]);
      return;
    }

That got me looking at $this->voidedItems, which is empty. So these were never "voided." While these transactions were never added to the voided item list, the docblock in ::voidStackItem() says:

    // The item should be removed from $stack and added to $voidedItems for
    // later processing.

There isn't a read accessor of $voidedItems that I can find - did I just discover a bug in the transaction manager, or am I just not familiar enough with this to know what I'm looking at?

Well... it turns out this is basically a dupe of 🐛 Transaction autocommit during shutdown relies on unreliable object destruction order Active .

Discussion in Slack is that this is indeed insidious and would be avoided in general by using a separate, non-transactional database connection a la 🐛 Race conditions with lock/cache using non-database storage - add a non-transactional database connection Needs work and deprecating/removing non-explicitly-committed database transactions at 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed

Closing Won't Fix (Fix Elsewhere.)