Fix FileTraceListener encoding to UTF-8 and fix DisposeAsync drain order#234
Merged
Conversation
- FileTraceListener StreamWriter now uses Encoding.UTF8 instead of Encoding.Default, preventing ArgumentException on Unicode characters (e.g., ➭ U+27AD). This was causing consumer task faults, channel fill, and OOM-killed pods when trace output contained non-ASCII. - DisposeAsync now sets _disposed AFTER waiting for consumer to drain. Previously _disposed=true was set before the drain, causing the consumer to see disposed=true and skip all events — leaving the file empty even when the fix was applied. - Add regression tests verifying UTF-8 encoding and consumer resilience under Unicode stress.
- Use UTF-8 without a BOM (UTF8Encoding(false)) so trace files don't get a stray EF BB BF prefix that confuses line-oriented log tooling. - Add a single-shot _disposeStarted guard so concurrent/repeated Dispose() and DisposeAsync() can't call _channel.Writer.Complete() twice (which throws). - Apply the same drain-order fix to the synchronous Dispose() path: it set _disposed=true and cancelled the consumer token BEFORE draining, so buffered events were skipped and the file came out empty. _disposed/_cts.Cancel() now happen after the drain, matching DisposeAsync. - Strengthen tests: sync-dispose now asserts flushed content, plus new no-BOM and redundant-disposal regression tests; temp files are cleaned up.
Owner
Author
|
Pushed follow-ups (commit 2ea0624) addressing three things found in review:
Full suite green (1143). Can't self-approve as author — flagging as ready. |
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
1 participant
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.
Problem
was using
StreamWriter(filePath, append: false)with Encoding.Default (system codepage). When trace output contained Unicode characters (e.g., ➭ U+27AD), the consumer task would throwArgumentException: Unable to translate Unicode character at index X to specified code page, crashing the consumer. With an unbounded channel, this would fill up and OOM-kill the pod.This is related to the earlier fix in PR #208, which addressed the same root cause (encoding mismatch) for TUI rendering (
Console.Out). This fix handles the trace logging path which was missed.Changes
FileTraceListener.csStreamWriternow explicitly usesEncoding.UTF8— same asAnsiTerminaldoes forConsole.Out_disposed = trueafter waiting for the consumer to drain the channel. Previously,_disposed=truewas set before the drain, causing the consumer to skip all events viaIsEnabled()— the file would be empty even if the consumer didn't crash.FileTraceListenerTests.cs(new)FileTraceListener_uses_UTF8_encoding_for_file_writers— writes Unicode content, disposes, verifies file contains all charactersFileTraceListener_consumer_handles_unicode_under_stress— pushes 100 Unicode events, verifies all 100 lines written (consumer didn't fault)StreamWriter_in_FileTraceListener_is_UTF8_not_Default— verifies dispose doesn't throw even with pure Unicode contentTesting