Skip to content

Commit 280b4ed

Browse files
authored
Fix minor AgentWriter bugs (#9013)
## Summary of changes Fixes two minor bugs, discovered while refactoring ## Reason for change Spotted the bugs while refactoring other things. They are unlikely to hit in production, but are worth fixing, and orthogonal to the rest of the refactoring ## Implementation details - `MessagePackSerializer.Serialize` writes to a _shared_ buffer. Given that the cache could well be used inside a _different_ call to `MessagePackSerializer.Serialize`, that could end up corrupting the cache. Instead, get the string bytes directly, which creates a new write-sized array instead. - The `_forceFlush` field is accessed from different threads, and is replaced from both. Need Volatile/Interlocked to ensure that you don't get a stale reference ## Test coverage Meh, I considered a test for the MessagePackStringCache.cs issue, but it's pretty niche, and would only be re-introduced if you change the code back, so figured the comment was sufficient. ## Other details Part of a stack
1 parent 34f116e commit 280b4ed

2 files changed

Lines changed: 6 additions & 5 deletions

File tree

tracer/src/Datadog.Trace/Agent/AgentWriter.cs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ internal void WriteWatermark(Action watermark, bool wakeUpThread = true)
289289

290290
private void RequestFlush()
291291
{
292-
_forceFlush.TrySetResult(default);
292+
Volatile.Read(ref _forceFlush).TrySetResult(default);
293293
}
294294

295295
private async Task FlushBuffersTaskLoopAsync()
@@ -306,8 +306,9 @@ private async Task FlushBuffersTaskLoopAsync()
306306

307307
if (_forceFlush.Task.IsCompleted)
308308
{
309-
_forceFlush = new TaskCompletionSource<bool>(TaskOptions);
310-
tasks[1] = _forceFlush.Task;
309+
var forceFlush = new TaskCompletionSource<bool>(TaskOptions);
310+
Volatile.Write(ref _forceFlush, forceFlush);
311+
tasks[1] = forceFlush.Task;
311312
}
312313

313314
await FlushBuffers().ConfigureAwait(false);

tracer/src/Datadog.Trace/Agent/MessagePack/MessagePackStringCache.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,8 +150,8 @@ public static void Clear()
150150
return localCachedBytes.Bytes;
151151
}
152152

153-
// encode the string into MessagePack and cache the bytes before returning them
154-
var bytes = string.IsNullOrWhiteSpace(value) ? null : MessagePackSerializer.Serialize(value);
153+
// Encode the string into MessagePack and cache the bytes before returning them.
154+
var bytes = string.IsNullOrWhiteSpace(value) ? null : MessagePackBinary.GetEncodedStringBytes(value!);
155155
cachedBytes = new CachedBytes(value, bytes);
156156
return bytes;
157157
}

0 commit comments

Comments
 (0)