Fix RichTextBox sink event loss during disposal - #41
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A critical disposal hang risk and dependency compatibility issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes RichTextBox sink event loss during disposal by draining buffered events, adding regression coverage, and updating targets and package metadata.
Changes:
- Flushes the final buffered snapshot during shutdown.
- Adds sink lifecycle regression coverage.
- Updates targets, dependencies, and package version.
File summaries
| File | Summary |
|---|---|
Serilog.Sinks.RichTextBox.WinForms.Colored/Sinks/RichTextBoxForms/RichTextBoxSink.cs |
Adds final flushing; cancellation handling may allow disposal to hang. |
Serilog.Sinks.RichTextBox.WinForms.Colored/Serilog.Sinks.RichTextBox.WinForms.Colored.csproj |
Updates package metadata; raises the Serilog minimum to 4.4.0 without documented justification. |
Serilog.Sinks.RichTextBox.WinForms.Colored.Test/Serilog.Sinks.RichTextBox.WinForms.Colored.Test.csproj |
Updates test tooling and dependencies. |
Serilog.Sinks.RichTextBox.WinForms.Colored.Test/Integration/SinkLifecycleTests.cs |
Adds disposal event-draining coverage. |
Demo/Demo.csproj |
Updates demo dependencies. |
Review details
Suppressed comments (1)
Serilog.Sinks.RichTextBox.WinForms.Colored/Sinks/RichTextBoxForms/RichTextBoxSink.cs:154
- This call still does not synchronously apply the final snapshot:
ProcessMessagesruns onTask.Run, andSetRtfusesBeginInvokewhenInvokeRequiredis true, so this returns after only queuing the UI delegate.Dispose()waits for_processingTaskbut not that delegate; if the caller disposes the control immediately afterward, the handle can be destroyed before the callback runs and the final events are still lost. Shutdown needs to acknowledge the final UI callback without blocking the UI thread, and this path needs a test that disposes the control immediately.
_richTextBox.SetRtf(builder.Rtf, _options.AutoScroll, stopping ? CancellationToken.None : token);
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vonhoff
force-pushed
the
maintenance
branch
from
September 12, 2026 18:41
d5e3e04 to
6070aff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The RichTextBox sink could display only the first of several rapidly emitted log events when the logger was immediately closed, especially when combined with the File sink.
This change drains the final buffered snapshot before stopping the sink’s processing task, preserving events emitted before disposal. It also adds regression coverage, and bumps the package version to 3.2.1