Performance: remove unnecessary Task.Run from payload unary interception#781
Open
berndverst wants to merge 2 commits into
Open
Performance: remove unnecessary Task.Run from payload unary interception#781berndverst wants to merge 2 commits into
berndverst wants to merge 2 commits into
Conversation
Fixes #774 Replace the Task.Run(async () => {...}) wrapper around request externalization + continuation invocation with a directly-invoked local async function. This avoids an unconditional thread-pool hop on every unary call when externalization completes synchronously (the common case), while preserving identical externalization ordering, exception propagation, cancellation, response headers, status/trailers, and disposal behavior. Add focused unit tests in Worker.Grpc.Tests covering the success path (ordering + response/headers/status/trailers), externalization failure, cancellation, and disposal (before completion and after failure), plus a regression test asserting externalization and the continuation run inline on the calling thread instead of being dispatched to the thread pool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42263041-0f76-4a63-baa9-e8fa73c35466
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes an unconditional Task.Run from the Azure Blob payload interceptor’s unary call path to eliminate a thread-pool hop on every gRPC unary call, while preserving the ordering and error/cancellation behavior of request externalization and continuation invocation.
Changes:
- Replaced
Task.Run(async () => ...)with a directly-invoked local async function (StartCallAsync) inPayloadInterceptor.AsyncUnaryCall. - Added unit tests validating ordering, exception/cancellation propagation, disposal behavior, and a regression guard ensuring synchronous externalization runs inline (no thread hop).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Extensions/AzureBlobPayloads/Interceptors/PayloadInterceptor.cs | Removes the Task.Run wrapper and starts the async externalization/continuation sequence inline to avoid a forced thread-pool dispatch. |
| test/Worker/Grpc.Tests/PayloadInterceptorTests.cs | Adds coverage for success/failure/cancellation/disposal flows and includes a regression test to ensure the call path remains inline when externalization is synchronous. |
Comment on lines
+180
to
+185
| externalizeGate.SetResult(); | ||
| await call.ResponseAsync; | ||
|
|
||
| // Assert: once the inner call exists, disposal is applied to it. | ||
| disposeInvoked.Should().BeTrue(); | ||
| } |
Dispose() schedules the inner call's disposal via a ContinueWith on startCallTask, which is a separate continuation from the one driving ResponseAsync(). The two continuations are not ordered relative to each other, so asserting disposeInvoked immediately after awaiting ResponseAsync could race and fail intermittently. Signal a TaskCompletionSource from the fake call's dispose action and await it (with a bounded 5s timeout) before asserting, so the test deterministically waits for Dispose's continuation instead of relying on incidental ordering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42263041-0f76-4a63-baa9-e8fa73c35466
Comment on lines
44
to
46
| // Externalize first; if this fails, do not proceed to send the gRPC call | ||
| await this.ExternalizeRequestPayloadsAsync(request, context.Options.CancellationToken); | ||
|
|
Comment on lines
+79
to
+98
| using AsyncUnaryCall<string> call = interceptor.AsyncUnaryCall( | ||
| "request", | ||
| context, | ||
| (req, ctx) => | ||
| { | ||
| order.Add("continuation"); | ||
| return CreateFakeCall("response", disposeAction: () => disposeInvoked = true); | ||
| }); | ||
| string response = await call.ResponseAsync; | ||
| Metadata headers = await call.ResponseHeadersAsync; | ||
|
|
||
| // Assert: ordering, response payload, and pass-through metadata are all preserved. | ||
| order.Should().Equal("externalize", "continuation", "resolve"); | ||
| response.Should().Be("response"); | ||
| headers.Should().NotBeNull(); | ||
| call.GetStatus().StatusCode.Should().Be(StatusCode.OK); | ||
| call.GetTrailers().Should().NotBeNull(); | ||
|
|
||
| call.Dispose(); | ||
| disposeInvoked.Should().BeTrue(); |
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.
Fixes #774
Summary
PayloadInterceptor.AsyncUnaryCallunconditionally wrapped request-payload externalization + continuation invocation inTask.Run(async () => {...}), forcing a thread-pool hop on every unary gRPC call even when externalization completes synchronously (the common case, since most requests don't exceed the large-payload threshold).This PR replaces the
Task.Runwrapper with a directly-invoked local async function (StartCallAsync()), removing the thread-pool dispatch while keeping the exact same internal sequence: externalize first, then invoke the continuation only if externalization succeeds.Why behavior is unchanged
OperationCanceledException→ faulted/canceled task) are identical whether the async method is invoked viaTask.Runor called directly — only the scheduling (inline vs. thread-pool) differs, not the resultingTaskstate.ResponseAsync,ResponseHeadersAsync,GetStatus,GetTrailers, andDisposeall continue to read from the samestartCallTask, untouched by this change.Tests
Added
test/Worker/Grpc.Tests/PayloadInterceptorTests.cs(home chosen because this project already has a conditionalProjectReferencetoAzureBlobPayloadsfor compatible target frameworks) covering:ResponseAsync, status reflects the failure.OperationCanceledExceptionpropagates, status isCancelled.Dispose()called before the inner call exists is safely deferred and applied once available.Dispose()is a no-op, does not throw.Task.Run-based implementation, confirming it's a meaningful regression guard).All 143 tests in
Worker.Grpc.Testspass locally.