refactor(sentry): improve timer cleanup in metrics listeners #1046
Conversation
Replace manual timer cleanup with Timer's built-in closure parameter support. This removes the need for separate coroutines to monitor WORKER_EXIT events and simplifies the timer handling logic across all metrics listeners. Changes: - Use Timer's closure parameter to handle cleanup gracefully - Remove manual coroutine creation and CoordinatorManager usage - Improve overall resource management and reduce complexity Affected listeners: - OnBeforeHandle - OnCoroutineServerStart - OnMetricFactoryReady - OnWorkerStart - PoolWatcher - QueueWatcher
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 演练该 PR 在六个指标监听器中重构了定时器关闭逻辑：将显式的协程等待与清理（通过 CoordinatorManager / Coroutine 等）移除，改为在定时器回调中接受 变更
评估代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟
可能相关的 PR
建议审查者
诗歌
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/src/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/*/src/**/*.php📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.php📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)src/sentry/src/Metrics/Listener/OnBeforeHandle.php (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (3)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.33)At least one path must be specified to analyse. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| $this->timer->tick($this->feature->getMetricsInterval(), function ($isClosing = false) use ($metrics, $event) { | ||
| if ($isClosing) { | ||
| return Timer::STOP; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (1)
58-62: 可选观察：$this->running标志永不重置。
$this->running标志在第 62 行设置为true，但在定时器停止后永远不会重置为false。对于MainCoroutineServerStart事件来说，这可能是有意为之的，因为该事件在服务器生命周期中应该只触发一次。但是，如果定时器因某种原因停止（通过返回
Timer::STOP），并且process()方法再次被调用，定时器将不会重新启动。如果这是预期行为，那么当前实现是正确的。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php(1 hunks)src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php(1 hunks)src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php(1 hunks)src/sentry/src/Metrics/Listener/OnWorkerStart.php(1 hunks)src/sentry/src/Metrics/Listener/PoolWatcher.php(1 hunks)src/sentry/src/Metrics/Listener/QueueWatcher.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: All PHP files must includedeclare(strict_types=1)at the top
Use PSR-12 coding standards for PHP code formatting
Use 4-space indentation with short array syntax in PHP code
Use.php-cs-fixer.phpconfiguration for PHP code formattingAll PHP files must include a strict_types declaration at the top
**/*.php: Follow project PHP coding standard enforced by php-cs-fixer with PSR-12 style, 4-space indentation, and short array syntax
Maintain type coverage by updating or adding tests when public APIs change; ensurecomposer test:typesstays green before pushing
Files:
src/sentry/src/Metrics/Listener/OnWorkerStart.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.phpsrc/sentry/src/Metrics/Listener/QueueWatcher.phpsrc/sentry/src/Metrics/Listener/OnMetricFactoryReady.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.phpsrc/sentry/src/Metrics/Listener/PoolWatcher.php
src/**/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use namespace pattern
FriendsOfHyperf\{ComponentName}for all component classes
Files:
src/sentry/src/Metrics/Listener/OnWorkerStart.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.phpsrc/sentry/src/Metrics/Listener/QueueWatcher.phpsrc/sentry/src/Metrics/Listener/OnMetricFactoryReady.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.phpsrc/sentry/src/Metrics/Listener/PoolWatcher.php
src/*/src/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
src/*/src/**/*.php: Namespace convention for components must follow FriendsOfHyperf{ComponentName}
All code must be coroutine-safe and avoid global state without proper context management, blocking I/O operations, and non-coroutine-safe third-party libraries without wrappers
Use Hyperf's Context API for request-scoped data instead of global state
Follows PSR-12 coding standards and use PHP-CS-Fixer for automatic formatting
Use PHPStan at maximum level for static analysis
Ensure component namespace doesn't conflict with existing Hyperf components or other packages in the ecosystem
Integrate deeply with Hyperf's Dependency Injection container for service registration
Leverage Hyperf's AOP (Aspect-Oriented Programming) for cross-cutting concerns via aspects defined in ConfigProvider
Use Hyperf's Event System to register listeners for framework events in components
All code must support coroutine-based concurrency using Swoole/Swow compatibility
Files:
src/sentry/src/Metrics/Listener/OnWorkerStart.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.phpsrc/sentry/src/Metrics/Listener/QueueWatcher.phpsrc/sentry/src/Metrics/Listener/OnMetricFactoryReady.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.phpsrc/sentry/src/Metrics/Listener/PoolWatcher.php
src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Each component lives in its own subdirectory with PSR-4 namespaces matching
FriendsOfHyperf\*
Files:
src/sentry/src/Metrics/Listener/OnWorkerStart.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.phpsrc/sentry/src/Metrics/Listener/QueueWatcher.phpsrc/sentry/src/Metrics/Listener/OnMetricFactoryReady.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.phpsrc/sentry/src/Metrics/Listener/PoolWatcher.php
🧠 Learnings (1)
📚 Learning: 2025-12-12T05:33:43.947Z
Learnt from: CR
Repo: friendsofhyperf/components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T05:33:43.947Z
Learning: Applies to src/*/src/**/*.php : Use Hyperf's Event System to register listeners for framework events in components
Applied to files:
src/sentry/src/Metrics/Listener/QueueWatcher.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
🧬 Code graph analysis (5)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (3)
src/sentry/src/Function.php (2)
feature(25-28)metrics(30-33)src/sentry/src/Feature.php (1)
getMetricsInterval(57-66)src/sentry/src/Metrics/Timer.php (1)
Timer(18-62)
src/sentry/src/Metrics/Listener/QueueWatcher.php (3)
src/sentry/src/Function.php (1)
feature(25-28)src/sentry/src/Feature.php (1)
getMetricsInterval(57-66)src/sentry/src/Metrics/Timer.php (1)
Timer(18-62)
src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (3)
src/sentry/src/Function.php (2)
feature(25-28)metrics(30-33)src/sentry/src/Feature.php (1)
getMetricsInterval(57-66)src/sentry/src/Metrics/Timer.php (1)
Timer(18-62)
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (3)
src/sentry/src/Function.php (2)
feature(25-28)metrics(30-33)src/sentry/src/Feature.php (1)
getMetricsInterval(57-66)src/sentry/src/Metrics/Timer.php (1)
Timer(18-62)
src/sentry/src/Metrics/Listener/PoolWatcher.php (3)
src/sentry/src/Function.php (1)
feature(25-28)src/sentry/src/Feature.php (1)
getMetricsInterval(57-66)src/sentry/src/Metrics/Timer.php (1)
Timer(18-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Agent
- GitHub Check: Seer Code Review
🔇 Additional comments (5)
src/sentry/src/Metrics/Listener/QueueWatcher.php (1)
55-58: 定时器关闭模式实现正确。与其他指标监听器保持一致的
$isClosing参数处理模式，简化了资源清理逻辑。基于 learnings，该文件正确使用了 Hyperf 的事件系统来注册监听器。src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)
90-93: 实现正确，与整体重构保持一致。定时器回调使用
$isClosing参数进行优雅关闭，移除了之前基于协程的清理逻辑，代码更加简洁。src/sentry/src/Metrics/Listener/PoolWatcher.php (1)
65-72: 抽象类中的定时器模式实现正确。
watch()方法中的定时器回调正确使用了$isClosing参数，与其他监听器保持一致。这使得所有继承此抽象类的具体连接池监控器都能受益于简化的清理机制。src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (1)
99-107: 重构方案清晰合理。使用
$isClosing参数和Timer::STOP返回值来处理定时器关闭是 Hyperf Coordinator Timer 的标准模式，相比之前基于协程监听WORKER_EXIT事件的方式更加简洁。Timer::STOP常量在 Hyperf 的协调器组件中明确定义，已在多个监听器中验证使用。代码充分利用
defer()确保协程安全的资源清理，并通过 Hyperf 的 Context API 避免全局状态。第 107 行新增的Timer::stats()指标采集是一个有价值的补充，可以监控定时器的运行状态。src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (1)
100-104: Timer 类的$isClosing参数模式已验证为正确支持。Hyperf 的
Timer::tick()方法完全支持将$isClosing参数传递给回调闭包。该实现在Hyperf\Coordinator\Timer类中明确定义（第 76 行），当$isClosing为 true 时，返回Timer::STOP会正确停止计时器。这个模式在整个项目中一致使用，包括PoolWatcher.php、OnWorkerStart.php、OnMetricFactoryReady.php等多个监听器类中。代码实现无误。
There was a problem hiding this comment.
Pull request overview
This PR attempts to refactor timer cleanup logic across Sentry metrics listeners by replacing the manual cleanup pattern (using CoordinatorManager and explicit timer->clear() calls) with a presumed built-in closure parameter feature. However, this refactoring appears to be based on a misunderstanding of how Hyperf's Coordinator\Timer::tick() method works.
Key changes:
- Adds
$isClosing = falseparameter to all timer tick callbacks - Removes
CoordinatorManager::until(Constants::WORKER_EXIT)wait pattern - Removes explicit
timer->clear($timerId)calls - Removes unused imports for
Constants,CoordinatorManager, andCoroutine(except whereCoroutineis still used forCoroutine::stats())
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sentry/src/Metrics/Listener/QueueWatcher.php | Refactored timer cleanup to use $isClosing parameter pattern, removed coordinator-based cleanup |
| src/sentry/src/Metrics/Listener/PoolWatcher.php | Refactored timer cleanup to use $isClosing parameter pattern, removed coordinator-based cleanup |
| src/sentry/src/Metrics/Listener/OnWorkerStart.php | Refactored timer cleanup to use $isClosing parameter pattern, removed coordinator-based cleanup |
| src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php | Refactored timer cleanup to use $isClosing parameter pattern, removed coordinator-based cleanup |
| src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php | Refactored timer cleanup to use $isClosing parameter pattern, removed coordinator-based cleanup |
| src/sentry/src/Metrics/Listener/OnBeforeHandle.php | Refactored timer cleanup to use $isClosing parameter pattern, removed coordinator-based cleanup |
Critical Issue Identified: The refactoring assumes that Hyperf\Coordinator\Timer::tick() automatically passes an $isClosing parameter to callbacks during shutdown. This doesn't appear to be a documented or implemented feature of Hyperf's Timer API. Other components in this repository (trigger, confd) use different patterns without this parameter. The original CoordinatorManager-based cleanup pattern is the established Hyperf approach for ensuring proper timer cleanup during worker shutdown.
| $this->timer->tick($this->feature->getMetricsInterval(), function ($isClosing = false) { | ||
| if ($isClosing) { | ||
| return Timer::STOP; | ||
| } |
There was a problem hiding this comment.
This refactoring assumes that Timer::tick() passes an $isClosing parameter to the callback during shutdown, but this doesn't appear to be how Hyperf's Coordinator\Timer works. The Timer::tick() method in Hyperf doesn't automatically invoke callbacks with a closing signal.
Looking at other usages in this codebase (trigger component's HealthMonitor, RedisServerMutex), they return Timer::STOP based on runtime conditions they check themselves, not on a parameter passed by the Timer.
The original pattern using CoordinatorManager::until(Constants::WORKER_EXIT) to wait for worker exit and then calling timer->clear() is the established Hyperf pattern for cleanup. Without this, the timers may not be properly cleaned up during shutdown, potentially causing memory leaks or preventing graceful shutdowns.
| $this->timer->tick($this->feature->getMetricsInterval(), function ($isClosing = false) use ( | ||
| $pool, | ||
| $workerId, | ||
| $poolName | ||
| ) { | ||
| if ($isClosing) { | ||
| return Timer::STOP; | ||
| } |
There was a problem hiding this comment.
This refactoring assumes that Timer::tick() passes an $isClosing parameter to the callback during shutdown, but this doesn't appear to be how Hyperf's Coordinator\Timer works. The Timer::tick() method in Hyperf doesn't automatically invoke callbacks with a closing signal.
The original pattern using CoordinatorManager::until(Constants::WORKER_EXIT) to wait for worker exit and then calling timer->clear() is the established Hyperf pattern for cleanup. Without this, the timers may not be properly cleaned up during shutdown, potentially causing memory leaks or preventing graceful shutdowns.
| $this->timer->tick($this->feature->getMetricsInterval(), function ($isClosing = false) use ($metrics, $event) { | ||
| if ($isClosing) { | ||
| return Timer::STOP; | ||
| } |
There was a problem hiding this comment.
This refactoring assumes that Timer::tick() passes an $isClosing parameter to the callback during shutdown, but this doesn't appear to be how Hyperf's Coordinator\Timer works. The Timer::tick() method in Hyperf doesn't automatically invoke callbacks with a closing signal.
The original pattern using CoordinatorManager::until(Constants::WORKER_EXIT) to wait for worker exit and then calling timer->clear() is the established Hyperf pattern for cleanup. Without this, the timers may not be properly cleaned up during shutdown, potentially causing memory leaks or preventing graceful shutdowns.
| $this->timer->tick($this->feature->getMetricsInterval(), function ($isClosing = false) use ($metrics, $serverStatsFactory, $workerId) { | ||
| if ($isClosing) { | ||
| return Timer::STOP; | ||
| } |
There was a problem hiding this comment.
This refactoring assumes that Timer::tick() passes an $isClosing parameter to the callback during shutdown, but this doesn't appear to be how Hyperf's Coordinator\Timer works. The Timer::tick() method in Hyperf doesn't automatically invoke callbacks with a closing signal.
The original pattern using CoordinatorManager::until(Constants::WORKER_EXIT) to wait for worker exit and then calling timer->clear() is the established Hyperf pattern for cleanup. Without this, the timers may not be properly cleaned up during shutdown, potentially causing memory leaks or preventing graceful shutdowns.
| $this->timer->tick($this->feature->getMetricsInterval(), function ($isClosing = false) use ($metrics) { | ||
| if ($isClosing) { | ||
| return Timer::STOP; | ||
| } |
There was a problem hiding this comment.
This refactoring assumes that Timer::tick() passes an $isClosing parameter to the callback during shutdown, but this doesn't appear to be how Hyperf's Coordinator\Timer works. The Timer::tick() method in Hyperf doesn't automatically invoke callbacks with a closing signal.
The original pattern using CoordinatorManager::until(Constants::WORKER_EXIT) to wait for worker exit and then calling timer->clear() is the established Hyperf pattern for cleanup. Without this, the timers may not be properly cleaned up during shutdown, potentially causing memory leaks or preventing graceful shutdowns.
* refactor(sentry): improve timer cleanup in metrics listeners Replace manual timer cleanup with Timer's built-in closure parameter support. This removes the need for separate coroutines to monitor WORKER_EXIT events and simplifies the timer handling logic across all metrics listeners. Changes: - Use Timer's closure parameter to handle cleanup gracefully - Remove manual coroutine creation and CoordinatorManager usage - Improve overall resource management and reduce complexity Affected listeners: - OnBeforeHandle - OnCoroutineServerStart - OnMetricFactoryReady - OnWorkerStart - PoolWatcher - QueueWatcher * refactor(metrics): add defer function import in OnBeforeHandle listener --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.