Move delayed transaction finisher state out of middleware (#936)

* Move delayed transaction finisher state out of middleware

* Update comment

Author: stayallive

src/Sentry/Laravel/Tracing/Middleware.php

@@ -47,13 +47,6 @@ class Middleware
      */
     private $continueAfterResponse;
 
-    /**
-     * Whether the terminating callback has been registered.
-     *
-     * @var bool
-     */
-    private $registeredTerminatingCallback = false;
-
     /**
      * Whether a defined route was matched in the application.
      *
@@ -115,22 +108,11 @@ public function terminate(Request $request, $response): void
         }
 
         if ($this->continueAfterResponse) {
-            // Ensure we do not register the terminating callback multiple times since there is no point in doing so
-            if ($this->registeredTerminatingCallback) {
-                return;
-            }
-
-            // We need to finish the transaction after the response has been sent to the client
-            // so we register a terminating callback to do so, this allows us to also capture
-            // spans that are created during the termination of the application like queue
-            // dispatched using dispatch(...)->afterResponse(). This middleware is called
-            // before the terminating callbacks so we are 99.9% sure to be the last one
-            // to run except if another terminating callback is registered after ours.
-            app()->terminating(function () {
-                $this->finishTransaction();
-            });
-
-            $this->registeredTerminatingCallback = true;
+            // Resolving the transaction finisher class will register the terminating callback
+            // which is responsible for calling `finishTransaction`. We have registered the
+            // class as a singleton to keep the state in the container and away from here
+            // this way we ensure the callback is only registered once even for Octane.
+            app(TransactionFinisher::class);
         } else {
             $this->finishTransaction();
         }
@@ -220,12 +202,12 @@ private function addAppBootstrapSpan(): ?Span
 
         $span = $this->transaction->startChild($spanContextStart);
 
-        // Consume the booted timestamp, because we don't want to report the bootstrap span more than once
-        $this->bootedTimestamp = null;
-
         // Add more information about the bootstrap section if possible
         $this->addBootDetailTimeSpans($span);
 
+        // Consume the booted timestamp, because we don't want to report the boot(strap) spans more than once
+        $this->bootedTimestamp = null;
+
         return $span;
     }
 
@@ -250,7 +232,7 @@ private function hydrateResponseData(SymfonyResponse $response): void
         $this->transaction->setHttpStatus($response->getStatusCode());
     }
 
-    private function finishTransaction(): void
+    public function finishTransaction(): void
     {
         // We could end up multiple times here since we register a terminating callback so
         // double check if we have a transaction before trying to finish it since it could

src/Sentry/Laravel/Tracing/ServiceProvider.php

@@ -59,6 +59,8 @@ public function boot(): void
 
     public function register(): void
     {
+        $this->app->singleton(TransactionFinisher::class);
+
         $this->app->singleton(Middleware::class, function () {
             $continueAfterResponse = ($this->getTracingConfig()['continue_after_response'] ?? true) === true;
 

src/Sentry/Laravel/Tracing/TransactionFinisher.php

@@ -0,0 +1,28 @@
+<?php
+
+namespace Sentry\Laravel\Tracing;
+
+/**
+ * @internal
+ */
+class TransactionFinisher
+{
+    public function __construct()
+    {
+        // We need to finish the transaction after the response has been sent to the client
+        // so we register a terminating callback to do so, this allows us to also capture
+        // spans that are created during the termination of the application like queue
+        // dispatches using dispatch(...)->afterResponse() which are terminating
+        // callbacks themselfs just like we do below.
+        //
+        // This class is registered as a singleton in the container to ensure it's only
+        // instantiated once and the terminating callback is only registered once.
+        //
+        // It should be resolved from the container before the terminating callbacks are called.
+        // Good place is in the `terminate` callback of a middleware for example.
+        // This way we can be 99.9% sure to be the last ones to run.
+        app()->terminating(function () {
+            app(Middleware::class)->finishTransaction();
+        });
+    }
+}