Skip to content

Skip logging errors on expected grpc client disconnect scenarios #11182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 18, 2025

Conversation

kshyju
Copy link
Member

@kshyju kshyju commented Jul 14, 2025

resolves #10572

Improves handling of gRPC stream disconnections by skipping error logging when the client disconnects in an expected manner during bidirectional streaming.

  • In gRPC bidirectional streaming, client disconnects are a common and expected behavior (e.g., process exit, network interruption).
  • When a disconnect occurs while the server is awaiting MoveNext(), it throws an IOException wrapping a ConnectionAbortedException.
  • Previously, all exceptions were logged as errors, including these expected disconnects.
  • This PR adds logic to detect this specific case and skip logging it as an error if ServerCallContext.CancellationToken has been canceled.

Repro note

The only way I could reproduce this behavior was by introducing an artificial delay in cancellation token signaling.

CancellationTokenRegistration ctr = cts.Token.Register(static state => ((TaskCompletionSource<bool>)state).TrySetResult(false), cancelSource);

Specifically, a Task.Delay(100) was added before completing the TaskCompletionSource to simulate a race condition.

CancellationTokenRegistration ctr = cts.Token.Register(static async state =>
{
    await Task.Delay(100);
    ((TaskCompletionSource<bool>)state).TrySetResult(false);
}, cancelSource);

This causes MoveNext() to enter before cancellation is signaled, triggering the exception in a realistic edge case.

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
    • Otherwise: Link to backporting PR
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

@kshyju kshyju requested a review from a team as a code owner July 14, 2025 15:48
@kshyju kshyju requested review from brettsam and Copilot July 14, 2025 15:49
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Improves handling of gRPC bidirectional streaming by ignoring expected client disconnect exceptions instead of logging them as errors.

  • Adds a specialized catch for IOException with an inner ConnectionAbortedException when the server call’s cancellation token is already canceled.
  • Imports the required namespaces (System.IO, Microsoft.AspNetCore.Connections) for the new exception handling.
  • Updates release_notes.md to document the change.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/WebJobs.Script.Grpc/Server/FunctionRpcService.cs Added a conditional catch block to suppress logging on expected client disconnect.
release_notes.md Noted the new behavior for skipping error logging on gRPC client disconnect.
Comments suppressed due to low confidence (2)

src/WebJobs.Script.Grpc/Server/FunctionRpcService.cs:79

  • This new error-handling path isn't covered by existing tests; consider adding a unit test that simulates a client disconnect to verify no error is logged when the cancellation token is canceled.
            catch (IOException ex) when (ex.InnerException is ConnectionAbortedException && context.CancellationToken.IsCancellationRequested)

src/WebJobs.Script.Grpc/Server/FunctionRpcService.cs:85

  • In an async method returning a boolean result, an empty return; may default to false or cause a compile error. Consider returning false explicitly to make the intention clear and ensure correct behavior.
                return;

@kshyju kshyju merged commit 0517532 into dev Jul 18, 2025
9 checks passed
@kshyju kshyju deleted the shkr/handle_grpc_client_connection_abort branch July 18, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Host can log a grpc ConnectionAbortedException during normal shutdown
3 participants