Repository navigation
Fix: send buffered events when process terminates - #380
buongarzoni wants to merge 6 commits into
Conversation
![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: 6050a31abc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (sender == null) { | ||
| return; | ||
| } | ||
| sender.close(true); |
There was a problem hiding this comment.
When shutdown begins while BufferedSender's scheduled daemon has already polled a payload and is still sending it, this close(true) call finds the queue empty and returns after BufferedSender.close() merely calls executorService.shutdown() without awaiting the active task. The hook then finishes, allowing the JVM to terminate the daemon mid-request and lose precisely the recent occurrence this change is intended to preserve. The shutdown path must also wait, within the configured timeout, for an already-running sender task to complete.
Useful? React with 👍 / 👎.
rollbar-circleci-machine
left a comment
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Overall: The change is sound. I read every changed file and its collaborators in the checkout and found no correctness defect in production code. I did not run the tests, so I can't say whether they pass.
What I checked:
- Hook lifecycle:
Rollbarregisters the hook only whenflushOnShutdown()is true and there is a sender (Rollbar.java:51-53,66-87).close(boolean)removes it before closing the sender (Rollbar.java:753-757). AnIllegalStateException(JVM already shutting down) and aSecurityExceptionare both caught on add and on remove (Rollbar.java:79-86,130-137). - Current sender at run time:
CurrentSenderProviderreads the live config underconfigReadLock(Rollbar.java:94-104). This matches howRollbarBase.configureswaps the config under the write lock (RollbarBase.java:141-153). - Timeout:
SenderShutdownHookflushes on a daemon thread and waits for it withjoin(timeoutMillis). It deliberately avoidsjoin(0), which would wait forever (SenderShutdownHook.java:52-74). BufferedSender.close(true): it now shuts down the executor and waits for any in-flight run before draining the queue (BufferedSender.java:129-147). That closes the real gap where a run had already taken payloads off the queue while they were still being sent. A periodic task is not rescheduled aftershutdown(), so the wait ends once the current run finishes.- Config: both new settings are carried through
withConfigandConfigImpl(ConfigBuilder.java:169-170,785-786). - Android: it opts out because
DiskQueuealready persists payloads (rollbar-android/.../Rollbar.java:489-493). - Style: the new Javadoc follows the existing
<p>style thatgoogle_checks.xmlalready accepts, and the new lines are within the 100-character limit.
Notes outside the findings (these are design or out-of-diff observations, not defects):
- Un-closed notifiers are kept alive: the registered hook holds a non-static inner
CurrentSenderProvider, which holds a reference to the wholeRollbar(Rollbar.java:71,94). Any notifier never passed toclose(boolean)is therefore kept until JVM exit. With the default config this adds little, because each un-closedBufferedSenderalready leaves a live executor thread behind. Apps that create manyRollbarinstances sharing one custom sender would now accumulate hooks, though. Consider saying in theflushOnShutdownJavadoc that notifiers should be closed. - The 2 s bound applies only to this PR's own hook: an explicit
close(true)is still unbounded (BufferedSender.java:141plusSyncSender, which sets no socket timeouts). The logback and log4j2 appenders call it fromstop()(rollbar-logback/.../RollbarAppender.java:140,rollbar-log4j2/.../RollbarAppender.java:145; neither file is in this diff). This was already true before the PR. - Busy loop while sending is suspended:
flushQueuespins when aSenderFailureStrategyhas sending suspended (BufferedSender.java:155-158,320-321). This was already true before the PR. The only implementation is Android's, and Android opts out of the hook. RollbarFilter.destroy()never closes the notifier (rollbar-web/.../RollbarFilter.java:92-95; not in this diff).
| public void shouldNotPropagateFlushFailures() throws Exception { | ||
| Sender sender = mock(Sender.class); | ||
| doThrow(new IllegalStateException("network down")).when(sender).close(true); | ||
|
|
||
| new SenderShutdownHook(providerOf(sender), TIMEOUT).run(); | ||
|
|
||
| verify(sender).close(true); | ||
| } |
There was a problem hiding this comment.
Low: This test can't fail, so it doesn't check what its Javadoc says. SenderShutdownHook.run() calls sender.close(true) on a separate rollbar-shutdown-flush thread (SenderShutdownHook.java:53-56,84). An exception thrown there can never reach the thread that called run(). If the catch (Exception) / catch (Throwable) blocks in FlushTask (SenderShutdownHook.java:85-91) were deleted, the IllegalStateException would go to the flush thread's uncaught-exception handler and this test would still pass. shouldDoNothingWhenThereIsNoSender (lines 117-123) has the same problem: it says 'No exception', but an NPE on the flush thread wouldn't be seen either.
2 participants
Description of the change
Type of change
Related issues
Checklists
Development
Code review