Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent registering terminating callback multiple times and incorrectly trying to finish transaction #717

Merged
merged 3 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# Changelog

## 3.5.1

The Sentry SDK team is happy to announce the immediate availability of Sentry Laravel SDK v3.5.1.

### Bug Fixes

- Prevent registering terminating callback multiple times and guard against finishing already finished transactions [(#717)](https://github.com/getsentry/sentry-laravel/pull/717)

This fixes the `Call to a member function finish() on null` that could occur when using long running processes like on the CLI or with Laravel Octane.

## 3.5.0

The Sentry SDK team is happy to announce the immediate availability of Sentry Laravel SDK v3.5.0.
Expand Down
21 changes: 21 additions & 0 deletions src/Sentry/Laravel/Tracing/Middleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ class Middleware
*/
private $app;

/**
* Whether the terminating callback has been registered.
*
* @var bool
*/
private $registeredTerminatingCallback = false;

/**
* Construct the Sentry tracing middleware.
*
Expand Down Expand Up @@ -102,6 +109,11 @@ public function terminate(Request $request, $response): void
if ($this->app === null) {
$this->finishTransaction();
} else {
// 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
Expand All @@ -111,6 +123,8 @@ public function terminate(Request $request, $response): void
$this->app->terminating(function () {
$this->finishTransaction();
});

$this->registeredTerminatingCallback = true;
}
}

Expand Down Expand Up @@ -223,6 +237,13 @@ private function hydrateResponseData(SymfonyResponse $response): void

private 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
// have already been finished in between being registered and being executed again
if ($this->transaction === null) {
return;
}

// Make sure we set the transaction and not have a child span in the Sentry SDK
// If the transaction is not on the scope during finish, the trace.context is wrong
SentrySdk::getCurrentHub()->setSpan($this->transaction);
Expand Down