Skip to content
Snippets Groups Projects

Transaction destruction with xdebug 3.3.0 Initial commit of #55 by @mondrake

Open Alex Pott requested to merge issue/drupal-3405976:3405976-mondrake-approach into 11.x
3 unresolved threads

Closes #3405976

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
287 292 */
288 293 public function __destruct() {
294 // Ensure all still-open transactions get auto-committed. Usually, this
295 // happens when the Transaction::__destruct() method is invoked, but during
296 // shutdown the object transaction order is unreliable. If the connection
297 // is destroyed first, we need to make sure to auto-commit all still-open
298 // transactions.
299 // Also see https://www.drupal.org/project/drupal/issues/1608374.
300 if ($this->isTransactionManagerStackActive) {
301 $this->commitTransactionOnDestruct();
302 }
303 else {
304 foreach (array_reverse($this->transactionLayers) as $name => $active) {
305 $this->popTransaction($name);
306 }
307 }
  • Comment on lines +303 to +307
    Author Maintainer
    Suggested change
    303 else {
    304 foreach (array_reverse($this->transactionLayers) as $name => $active) {
    305 $this->popTransaction($name);
    306 }
    307 }
    303 // Ensure the deprecation layer does not have any transactions.
    304 foreach (array_reverse($this->transactionLayers) as $name => $active) {
    305 $this->popTransaction($name);
    306 }
  • Oh yeay, but wouldn't it make sense to check there's no TransactionManager active anyway, to avoid exercising a deprecated path uselessly?

  • Please register or sign in to reply
  • mondrake @mondrake started a thread on the diff
  • 286 291 * Ensures that the client connection can be garbage collected.
    287 292 */
    288 293 public function __destruct() {
    294 // Ensure all still-open transactions get auto-committed. Usually, this
    295 // happens when the Transaction::__destruct() method is invoked, but during
    296 // shutdown the object transaction order is unreliable. If the connection
    297 // is destroyed first, we need to make sure to auto-commit all still-open
    298 // transactions.
    299 // Also see https://www.drupal.org/project/drupal/issues/1608374.
    300 if ($this->isTransactionManagerStackActive) {
    301 $this->commitTransactionOnDestruct();
    302 }
    303 else {
    304 foreach (array_reverse($this->transactionLayers) as $name => $active) {
  • daffie
    daffie @daffie started a thread on the diff
  • 1691 1722 }
    1692 1723 }
    1693 1724
    1725 /**
    1726 * Commit on destruction if a client transaction is still active.
    1727 *
    1728 * This method should only be called by __destruct().
    1729 *
    1730 * @internal
    1731 */
    1732 protected function commitTransactionOnDestruct(): void {
    1733 $this->connection->commit();
    • Can we not call commit() directly, but instead do this from the transaction manager.

    • I think issue is that end of script destruction is not in a determined order so the transaction manager might already have been destroyed and be null.

      Likewise if you try this from transaction manager the connection object isn’t guaranteed to be around and could already have been destroyed and therefore be null

    • It looks like the Connection object can have a reference to the transaction manager, which means it's not being destroyed, right? So if ($this->$transactionManager) { ... then use it, otherwise call connection->commit(). We'd look at the property rather than calling the service method ($this->transactionManager()) because otherwise we run into to the problem mentioned... The service method could end up generating a new transaction manager. Ideal solution: Actually inject the service instead of lazily generating one, so we always have a reference to it and it's not being destroyed before the Connection object.

    • Actually inject the service instead of lazily generating one, so we always have a reference to it and it's not being destroyed before the Connection object.

      Destruct doesn’t happen in order during script shutdown so even if you had a reference to something it might have been destroyed already and become NULL.

      It does now make me think you can’t actually guarantee the connection object itself is still available. Feels like all bets are off in this one.

      Edited by Jason Woods
    • Please register or sign in to reply
  • James Glasgow added 20 commits

    added 20 commits

    • c647edfa...3df04e71 - 19 commits from branch project:11.x
    • 462a84e3 - Merge remote-tracking branch 'origin/11.x' into 3405976-mondrake-approach

    Compare with previous version

  • James Glasgow added 321 commits

    added 321 commits

    Compare with previous version

  • Please register or sign in to reply
    Loading