Skip to content

Commit b18aab8

Browse files
authored
Change AgentWriter to only flush from a single task (#9007)
## Summary of changes - Changes `AgentWriter to only ever flush from the flush thread - Avoids dropping traces because both buffers are locked when manually flushing ## Reason for change Previously, if you called `FlushTracesAsync()` this would execute a flush on the _same_ thread that requested it (kinda, not really because `async`, but you get the idea). During a flush, the buffer being flushed is locked, so that the serializer thread doesn't write to it. If a background flush was running at the _same_ time as the manual flush request (or if you called `FlushTracesAsync()` twice concurrently) then you could end up locking _both_ the front and back buffers. Any attempts to serialize traces in this time would just drop them. > This isn't theoretical, recent additional logging, shows that we hit this scenario in CI with moderate frequency, and is one source of flake. Instead, this PR enforces these requirements: 1. All flushes go through the flush thread/task - That means that at most one thread is flushing at a time, which means _at most_ one buffer is locked at a time - It's still possible to drop traces, if one buffer is full, and the _other_ buffer is locked because it's being flushed. - If we want to add more resilience there, we could do that later, by adding more buffers/more queue readers/etc, but seems overkill without more information. 2. Any calls to `FlushTracesAsync()` (or `FlushAndCloseAsync()` flush any traces previously written by a `WriteTrace()` call in the same control flow. - This held before, but is an important invariant to specifiy 3. If you call `FlushTracesAsync()` multiple times, then the flush calls are "batched" to avoid multiple (pointless) flushes - I nicety really, we _could_ remove this requirement if we want ## Implementation details - `_frontBufferFlushTask` and `_backBufferFlushTask` are gone - `_pendingFlushRequest` is a way to hand out the same `Task` object to multiple concurrent callers of `FlushTracesAsync()`, so that they're "batched" - `FlushAndCloseAsync` timeout reduced from "up to 40s" to "up to 30s". Not necessary, just seemed reasonable - `FlushAndCloseAsync` doesn't need to do a final `Task.Run()` anymore - `FlushTracesAsync` no longer runs the flushing itself immediately, instead it just wakes the flush loop, and registers itself to be notified once it's completed - If the flush/serialize loop has already exited (during shutdown) the flush will complete, though nothing extra actually was flushed. - I didn't think it was necessary to distinguish these cases (e.g. by returning a `Task<bool>` , but we _could_ if there's a need - A couple of method renames to be clearer ## Test coverage Updated existing tests to handle the new behaviour, and added some new unit tests to cover the new behaviour. Will also keep an eye on CI for any flake and ensure that the "all locked" gap we were worried about is fixed ## Other details Part of a stack
1 parent a099858 commit b18aab8

2 files changed

Lines changed: 318 additions & 132 deletions

File tree

0 commit comments

Comments
 (0)