Skip to content

refactor(sentry): optimize SingletonAspect with match expression #1049

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.

Sign up for GitHub

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

Merged
huangdijia merged 4 commits into from
Dec 22, 2025
Merged

refactor(sentry): optimize SingletonAspect with match expression #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
450ebcc
refactor(sentry): optimize SingletonAspect with match expression for …
huangdijia Dec 22, 2025
ff15010
refactor(sentry): replace Co::defer with defer function for coroutine…
huangdijia Dec 22, 2025
1423d0c
fix(sentry): correct condition check for argument existence in Single…
huangdijia Dec 22, 2025
384465b
refactor(sentry): clean up SingletonAspect by commenting out unused s…
huangdijia Dec 22, 2025
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
6 changes: 4 additions & 2 deletions src/sentry/src/Aspect/CoroutineAspect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
use Sentry\SentrySdk;
use Throwable;

use function Hyperf\Coroutine\defer;

class CoroutineAspect extends AbstractAspect
{
public const CONTEXT_KEYS = [
Expand Down Expand Up @@ -58,7 +60,7 @@ protected function handleCreate(ProceedingJoinPoint $proceedingJoinPoint): void
}

// Defer the flushing of events until the coroutine completes.
Co::defer(fn () => Integration::flushEvents());
defer(fn () => Integration::flushEvents());

// Continue the callable in the new Coroutine.
$callable();
Expand All @@ -73,6 +75,6 @@ protected function handlePrintLog(ProceedingJoinPoint $proceedingJoinPoint): voi
return;
}

Co::defer(fn () => SentrySdk::getCurrentHub()->captureException($throwable));
defer(fn () => SentrySdk::getCurrentHub()->captureException($throwable));
}
}
60 changes: 33 additions & 27 deletions src/sentry/src/Aspect/SingletonAspect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,44 +19,50 @@
class SingletonAspect extends AbstractAspect
{
public array $classes = [
\Sentry\CheckInStatus::class . '::getInstance',
\Sentry\EventType::class . '::getInstance',
\Sentry\MonitorScheduleUnit::class . '::getInstance',
// Singleton Classes
\Sentry\State\HubAdapter::class . '::getInstance',
\Sentry\Integration\IntegrationRegistry::class . '::getInstance',
\Sentry\Logs\LogLevel::class . '::getInstance',
\Sentry\Metrics\TraceMetrics::class . '::getInstance',
\Sentry\State\HubAdapter::class . '::getInstance',
\Sentry\Tracing\SpanStatus::class . '::getInstance',
\Sentry\Tracing\TransactionSource::class . '::getInstance',
\Sentry\Transport\ResultStatus::class . '::getInstance',
\Sentry\Unit::class . '::getInstance',
// Enums
// \Sentry\CheckInStatus::class . '::getInstance',
// \Sentry\EventType::class . '::getInstance',
// \Sentry\MonitorScheduleUnit::class . '::getInstance',
// \Sentry\Logs\LogLevel::class . '::getInstance',
// \Sentry\Tracing\SpanStatus::class . '::getInstance',
// \Sentry\Tracing\TransactionSource::class . '::getInstance',
// \Sentry\Transport\ResultStatus::class . '::getInstance',
// \Sentry\Unit::class . '::getInstance',
];

public function process(ProceedingJoinPoint $proceedingJoinPoint)
{
$key = $className = $proceedingJoinPoint->className;
$arguments = $proceedingJoinPoint->getArguments();

if (isset($arguments[0])) {
if (! array_key_exists(0, $arguments)) {
Copy link

@sentry sentry bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The condition ! array_key_exists(0, $arguments) is the inverse of the previous logic. It incorrectly executes the following block when no arguments are passed, causing an error.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The logic to check for the existence of arguments in singleton getInstance methods has been inverted. The new condition if (! array_key_exists(0, $arguments)) evaluates to true when no arguments are provided. This causes the code to attempt to access $arguments[0] on an empty array, which will trigger a PHP "undefined array key" warning or error on every call to methods like TraceMetrics::getInstance(). This breaks the intended caching behavior.

💡 Suggested Fix

The conditional logic should be corrected to check for the existence of the argument before attempting to access it. Change if (! array_key_exists(0, $arguments)) to if (array_key_exists(0, $arguments)) to correctly append the argument to the cache key only when it is present.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/src/Aspect/SingletonAspect.php#L42

Potential issue: The logic to check for the existence of arguments in singleton
`getInstance` methods has been inverted. The new condition `if (! array_key_exists(0,
$arguments))` evaluates to `true` when no arguments are provided. This causes the code
to attempt to access `$arguments[0]` on an empty array, which will trigger a PHP
"undefined array key" warning or error on every call to methods like
`TraceMetrics::getInstance()`. This breaks the intended caching behavior.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7807594

$key .= '#' . $arguments[0];
}

return Context::getOrSet($key, function () use ($proceedingJoinPoint, $className, $arguments) {
// Reset singleton instance before proceeding
Closure::bind(function () use ($className, $arguments) {
if (property_exists($className, 'instance')) {
$className::$instance = null;
} elseif (
property_exists($className, 'instances')
&& isset($arguments[0])
&& array_key_exists($arguments[0], $className::$instances)
) {
$className::$instances[$arguments[0]] = null;
}
}, null, $className)();

// Proceed to get the singleton instance
return $proceedingJoinPoint->process();
});
return match ($className) {
// Singleton Classes
\Sentry\State\HubAdapter::class,
\Sentry\Integration\IntegrationRegistry::class => Context::getOrSet($key, function () use ($className) {
return Closure::bind(fn () => new $className(), null, $className)();
}),
\Sentry\Metrics\TraceMetrics::class => Context::getOrSet($key, function () use ($className) {
return new $className();
}),

// Enums
// \Sentry\CheckInStatus::class,
// \Sentry\EventType::class,
// \Sentry\MonitorScheduleUnit::class,
// \Sentry\Logs\LogLevel::class,
// \Sentry\Tracing\SpanStatus::class,
// \Sentry\Tracing\TransactionSource::class,
// \Sentry\Transport\ResultStatus::class,
// \Sentry\Unit::class => $proceedingJoinPoint->process(),
default => $proceedingJoinPoint->process(),
};
}
}
4 changes: 2 additions & 2 deletions src/sentry/src/Metrics/Aspect/HistogramAspect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
use FriendsOfHyperf\Sentry\Metrics\Timer;
use Hyperf\Di\Aop\AbstractAspect;
use Hyperf\Di\Aop\ProceedingJoinPoint;
use Hyperf\Engine\Coroutine as Co;

use function Hyperf\Coroutine\defer;
use function Hyperf\Tappable\tap;

class HistogramAspect extends AbstractAspect
Expand Down Expand Up @@ -54,7 +54,7 @@ public function process(ProceedingJoinPoint $proceedingJoinPoint): mixed
]);

return tap($proceedingJoinPoint->process(), function () use ($timer) {
Co::defer(fn () => $timer->end(true));
defer(fn () => $timer->end(true));
});
}

Expand Down
5 changes: 3 additions & 2 deletions src/sentry/src/Metrics/Listener/RequestWatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@
use FriendsOfHyperf\Sentry\Feature;
use FriendsOfHyperf\Sentry\Metrics\CoroutineServerStats;
use FriendsOfHyperf\Sentry\Metrics\Timer;
use Hyperf\Engine\Coroutine as Co;
use Hyperf\Event\Contract\ListenerInterface;
use Hyperf\HttpServer\Event as HttpEvent;
use Hyperf\HttpServer\Router\Dispatched;
use Hyperf\RpcServer\Event as RpcEvent;
use Psr\Http\Message\ServerRequestInterface;

use function Hyperf\Coroutine\defer;

class RequestWatcher implements ListenerInterface
{
public function __construct(
Expand Down Expand Up @@ -56,7 +57,7 @@ public function process(object $event): void
'request_method' => $request->getMethod(),
]);

Co::defer(function () use ($timer) {
defer(function () use ($timer) {
++$this->stats->close_count;
++$this->stats->response_count;
--$this->stats->connection_num;
Expand Down
3 changes: 2 additions & 1 deletion src/sentry/src/Tracing/Aspect/CoroutineAspect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

use function FriendsOfHyperf\Sentry\startTransaction;
use function FriendsOfHyperf\Sentry\trace;
use function Hyperf\Coroutine\defer;
use function Sentry\continueTrace;

class CoroutineAspect extends AbstractAspect
Expand Down Expand Up @@ -82,7 +83,7 @@ function (Scope $scope) use ($proceedingJoinPoint, $callingOnFunction) {
);

// Defer the finishing of the transaction and flushing of events until the coroutine completes.
Co::defer(function () use ($transaction) {
defer(function () use ($transaction) {
$transaction->finish();
Integration::flushEvents();
});
Expand Down
12 changes: 6 additions & 6 deletions src/sentry/src/Tracing/Listener/EventHandleListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
use Hyperf\Crontab\Event as CrontabEvent;
use Hyperf\Database\Events as DbEvent;
use Hyperf\DbConnection\Pool\PoolFactory;
use Hyperf\Engine\Coroutine as Co;
use Hyperf\Event\Contract\ListenerInterface;
use Hyperf\HttpServer\Event as HttpEvent;
use Hyperf\HttpServer\Router\Dispatched;
Expand All @@ -55,6 +54,7 @@

use function FriendsOfHyperf\Sentry\startTransaction;
use function FriendsOfHyperf\Sentry\trace;
use function Hyperf\Coroutine\defer;
use function Sentry\continueTrace;

/**
Expand Down Expand Up @@ -328,7 +328,7 @@ protected function handleRequestReceived(HttpEvent\RequestReceived|RpcEvent\Requ

SentrySdk::getCurrentHub()->setSpan($span);

Co::defer(function () use ($transaction, $span) {
defer(function () use ($transaction, $span) {
// Make sure the span is finished after the request is handled
$span->finish();

Expand Down Expand Up @@ -531,7 +531,7 @@ protected function handleCrontabTaskStarting(CrontabEvent\BeforeExecute $event):
])
);

Co::defer(function () use ($transaction) {
defer(function () use ($transaction) {
// Make sure the transaction is finished after the task is executed
SentrySdk::getCurrentHub()->setSpan($transaction);

Expand Down Expand Up @@ -600,7 +600,7 @@ protected function handleAmqpMessageProcessing(AmqpEvent\BeforeConsume $event):
])
);

Co::defer(function () use ($transaction) {
defer(function () use ($transaction) {
// Make sure the transaction is finished after the message is processed
SentrySdk::getCurrentHub()->setSpan($transaction);

Expand Down Expand Up @@ -668,7 +668,7 @@ protected function handleKafkaMessageProcessing(KafkaEvent\BeforeConsume $event)
])
);

Co::defer(function () use ($transaction) {
defer(function () use ($transaction) {
// Make sure the transaction is finished after the message is processed
SentrySdk::getCurrentHub()->setSpan($transaction);

Expand Down Expand Up @@ -718,7 +718,7 @@ protected function handleAsyncQueueJobProcessing(AsyncQueueEvent\BeforeHandle $e
])
);

Co::defer(function () use ($transaction) {
defer(function () use ($transaction) {
// Make sure the transaction is finished after the job is processed
SentrySdk::getCurrentHub()->setSpan($transaction);

Expand Down
3 changes: 2 additions & 1 deletion src/sentry/src/Tracing/Tracer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Sentry\Tracing\TransactionSource;
use Throwable;

use function Hyperf\Coroutine\defer;
use function Sentry\trace;

class Tracer
Expand All @@ -34,7 +35,7 @@ public function startTransaction(TransactionContext $transactionContext, array $
$hub->pushScope();
$hub->configureScope(static fn (Scope $scope) => $scope->clearBreadcrumbs());

Co::defer(static fn () => $hub->popScope());
defer(static fn () => $hub->popScope());

$transactionContext->setData(['coroutine.id' => Co::id()] + $transactionContext->getData());

Expand Down