Fix corrupted bytes sent after GOAWAY - #2766
Conversation
|
|
|
This looks like it creates a new serialization context every time. That will hurt performance of streaming calls. Instead of reusing a context for each streamed message, a new context is created. In ASP.NET Core we reuse a HttpContext if it completed successfully, otherwise it is thrown away. Can the same thing be done here? If there is an error then don't reuse the serialization context. |
JamesNK
left a comment
There was a problem hiding this comment.
The correctness fix is validated: the focused concurrent-write test fails on the base commit (Expected: 1, But was: 2) and passes on this head, and the complete GrpcCallSerializationContextTests fixture passes (18/18). I also ran the issue #2760 GOAWAY reproduction against both versions: base detected corrupted payload bytes, while this head completed 100 iterations with at least 38,000 retried payloads verified and no corruption.
The remaining issue before merge is the streaming allocation regression noted in my earlier comment. PR #1033 deliberately introduced serialization-context reuse for streaming; this change replaces one context per call with one context per message. Please preserve reuse for the sequential path while isolating overlaps. A per-call single-slot lease is sufficient: atomically take the cached context, allocate only when the slot is already leased, reset after use, and atomically return at most one clean context. That avoids a global pool and timeout cleanup while retaining the concurrency fix.
|
One way to preserve reuse without sharing an active context is a per- For example, the helper could look roughly like this: private GrpcCallSerializationContext? _serializationContext;
internal SerializationContextLease RentSerializationContext(CallOptions callOptions)
{
var context = Interlocked.Exchange(ref _serializationContext, null)
?? new GrpcCallSerializationContext(this);
try
{
context.CallOptions = callOptions;
context.Initialize();
return new SerializationContextLease(this, context);
}
catch
{
context.Reset();
throw; // Don't cache a context after initialization fails.
}
}
private void ReturnSerializationContext(GrpcCallSerializationContext context)
{
// Keep at most one context. A concurrent extra is left for GC.
Interlocked.CompareExchange(ref _serializationContext, context, null);
}
internal struct SerializationContextLease : IDisposable
{
private readonly GrpcCall _call;
private GrpcCallSerializationContext? _context;
private bool _reusable;
internal SerializationContextLease(GrpcCall call, GrpcCallSerializationContext context)
{
_call = call;
_context = context;
_reusable = false;
}
public readonly GrpcCallSerializationContext Context => _context!;
public void MarkReusable() => _reusable = true;
public void Dispose()
{
var context = _context;
if (context == null)
{
return;
}
_context = null;
context.Reset(); // Always release the rented payload buffer.
if (_reusable)
{
_call.ReturnSerializationContext(context);
}
}
}A call site then contains only the operation-specific work: var lease = call.RentSerializationContext(callOptions);
try
{
serializer(message, lease.Context);
await stream.WriteAsync(
lease.Context.GetWrittenPayload(),
call.CancellationToken).ConfigureAwait(false);
lease.MarkReusable();
}
finally
{
lease.Dispose();
}This gives us one allocation for the normal sequential streaming path, independent contexts for overlapping replay writes, at most one retained context after concurrent successes, and no reuse following serialization, initialization, or write errors. The same helper can be used by the retry and WinHTTP serialization paths. I would avoid a helper that accepts |
|
Thanks @JamesNK! I applied your suggestion, and included tests. It will definitely help reduce amount of allocations specifically for the non-contested paths where we would reuse same context over and over again. |
JamesNK
left a comment
There was a problem hiding this comment.
The follow-up resolves the streaming allocation concern while keeping overlapping serialization attempts isolated. The single-slot lease is removed atomically while active, failed operations are not cached, and retry payloads are copied before the context is returned.
Validation on aff70593:
Grpc.Net.Clientbuilds successfully for all six target frameworks with no warnings or errors.- Focused tests pass on .NET 10 (89/89) and .NET Framework 4.6.2 (57/57), including sequential reuse, overlapping leases, failed-reuse behavior, concurrent payload independence, retry, streaming, and compression coverage.
- A public
GrpcChannelclient-streaming probe validated 5,200 framed messages with no corruption; 200 concurrent unary messages also validated with no corruption. - The same 5,000-message streaming probe allocated about 127.9 fewer bytes per message than
2196f66, confirming the per-messageGrpcCallSerializationContextallocation has been removed.
The earlier issue #2760 GOAWAY stress result remains valid for the unchanged isolation behavior (100 iterations and at least 38,000 retried payloads without corruption). The harness was not available to rerun for this follow-up; native WinHTTP was also not exercised directly.
GrpcCallcached a singleGrpcCallSerializationContextinstance for the lifetime of the call. When the HTTP/2 transport replays a request onto a new connection (e.g. after receiving GOAWAY) while the original write is still in flight, both attempts can concurrently reinitialize and mutate the same cached context/buffer, corrupting the serialized message bytes on the wire. Current PR replaces the cached context with a newGrpcCallSerializationContextinstance so every serialization attempt gets its own instance, eliminating the shared mutable state.While I understand y-meh's comment:
I did not find a nice way to reuse same serialization contexts across different
GrpcCalls. The only way I could think of it is to create a pool, which will make us borrow from it instead of making a new allocation, but comes with more complications (like cleaning up pool by timeouts, etc). CurrentlyGrpcCallSerializationContextholdsbyte[] bufferbut it is ArrayPool-rented, so the cost of allocation is practically not costly.Fixes #2760