fix(sentry): make transport push non-blocking and resilient to consumer failure - #1088
fix(sentry): make transport push non-blocking and resilient to consumer failure#1088huangdijia wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
040e575 to
3150e50
Compare
3150e50 to
eb174e7
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 040e5752b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } finally { | ||
| $this->closeChannel(); |
There was a problem hiding this comment.
When the client is flushed and then reused—as happens in EventHandleListener.php:158 and RuntimeContextManager.php:210—close() closes and nulls channel A, allowing the next send() to install channel B while A's consumer coroutine unwinds. This finally then calls closeChannel(), which reads the current $this->chan rather than the channel owned by that consumer, so the old consumer can close and null B and cause subsequent events to be skipped; capture each consumer's channel and only clear it when it is still current.
Useful? React with 👍 / 👎.
| // The max seconds to wait when pushing an event into the transport channel | ||
| // `<= 0` means non-blocking (skip the event immediately when the channel is full), | ||
| // `> 0` means wait at most N seconds for the channel to have capacity. | ||
| 'transport_timeout' => (float) env('SENTRY_TRANSPORT_TIMEOUT', 1.0), |
There was a problem hiding this comment.
This introduces SENTRY_TRANSPORT_TIMEOUT, changes its default to one second, and gives non-positive values important non-blocking semantics, but the Transport sections inspected in both component READMEs and all four locale pages still list only channel size, concurrency, and HTTP timeout. Users following those guides therefore cannot discover how to select the new behavior; update all six Sentry documents together.
AGENTS.md reference: AGENTS.md:L129-L130
Useful? React with 👍 / 👎.
1 participant
问题
CoHttpTransport通过有限容量 Channel(发布配置默认 512)缓冲事件,但存在两类永久阻塞风险,会导致请求协程无法结束、协程数量无界堆积 → OOM:send()中$chan?->push($event, $this->timeout)在通道满时,若transport_timeout <= 0,内部超时保持-1(永久阻塞)。请求协程在协程结束时 flush 会卡死,协程数无界增长。loop()里makeHttpTransport()在 try 之外,一旦抛异常消费协程直接死亡,channel 永远无人消费,所有后续send()永久阻塞。close()为空实现,不会等待通道排空。修改
sentry.transport_timeout,<= 0时内部超时为0(非阻塞,通道满立即跳过事件),> 0时最多等待 N 秒。超时计算提取为 protected 方法resolvePushTimeout()。Channel::push($data, 0)实际会被当作"无限等待"(源码wait_push仅在timeout > 0时注册定时器),因此pushEvent()在非阻塞模式下先用isFull()短路跳过,并以极小正超时(0.001s)兜底竞态,保证调用方协程绝不挂起。loop()消费协程主体(makeHttpTransport、pop、派发)包进try/catch(Throwable),catch 中通过$this->clientBuilder?->getLogger()记录日志,finally中closeChannel();closeChannel()幂等,通道关闭后send()自愈(loop()检测 chan 为 null 时重建)。close()支持排空:等待通道排空(最多$timeout秒,默认 1 秒,while (! $chan->isEmpty()) msleep(...)+ 超时判断,与 workerWatcher 风格一致),然后closeChannel()返回 success;通道不存在时直接 success。clientBuilder的 logger,否则通过容器解析StdoutLoggerInterface,容器解析失败则忽略)。publish/sentry.php中transport_timeout默认值改为1.0,并注释说明<= 0表示非阻塞、> 0表示最多等待 N 秒;channel_size/concurrent_limit及其余配置不变。测试
新增
tests/Sentry/CoHttpTransportTest.php(Pest,Swoole\Coroutine\run包裹,通过子类覆盖loop()避免真实协程与 HTTP 发送),覆盖:resolvePushTimeout决策:<= 0 → 0、> 0 → 原值;transport_timeout <= 0映射为非阻塞超时;send()不阻塞并返回skipped;send()返回skipped;send()返回success;close()在通道有积压时等待排空后关闭,不阻塞超时;close()在通道不存在时直接 success。结果:
vendor/bin/pest tests/Sentry # 50 passed (88 assertions),连续多次运行一致说明:worktree 的
vendor是指向主 checkout 的符号链接,其 composer autoloader 会把FriendsOfHyperf\Sentry\*解析到主 checkout 源码,且全量加载时存在预置的TestEnum重复声明问题(与本次改动无关),故以tests/Sentry目录方式运行(与--group=sentry覆盖相同的 43 个既有用例 + 7 个新增)。测试文件顶部显式require_onceworktree 的CoHttpTransport.php以测试本地改动。验证
vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.php --dry-run --diff(仅本次 3 个文件):0 处待修复。git diff --check:无错误。composer analyse src/sentry(PHPStan):按要求未运行。