Skip to content
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

Handling faulted background task - revised proposal #2351

Closed
bernardnormier opened this issue Dec 20, 2022 · 2 comments · Fixed by #2632
Closed

Handling faulted background task - revised proposal #2351

bernardnormier opened this issue Dec 20, 2022 · 2 comments · Fixed by #2632
Labels
proposal Proposal for a new feature or significant update
Milestone

Comments

@bernardnormier
Copy link
Member

bernardnormier commented Dec 20, 2022

This proposal replaces #2231.

  1. remove ConnectionOptions.FaultedTaskAction

  2. the normal pattern for handling unexpected exception in a background task is as follows:

catch-and-eat expected exception..
catch (Exception exception) // unexpected
{
    Debug.Fail($"message with {exception}");
     // if Debug.Fail is no-op, we want this task to complete with an unobserved exception
     // that the UnobservedTaskException event can see
    throw;
}

When a logger (see below) is available, we also log this exception between the Debug.Fail and the throw:

catch (Exception exception) // unexpected
{
    Debug.Fail($"message with {exception}");
     _logger.LogXxxFailed(exception); // at the error level
    throw;
  1. For "unexpected" exceptions thrown by application code (such as Payload.ReadAsync) during:
  • the sending of a response (including the payload continuation of the response)
  • the sending of a request payload continuation

that are not otherwise reported, we log them and "eat" them:

catch (Exception exception) // unexpected "application exception"
{
    _logger.LogPayloadContinuationSendFailed(exception); // at the Debug level
    // exception is handled/consumed
}

We also use this logger to log otherwise unreported failures such as:

  • failure to decode a request header (Debug level)
  • response header too large to send (Warning level)
  • failure to encode a response header field (Warning level)
  • bad flush result from pipe writer (Error level)
@bernardnormier bernardnormier added the proposal Proposal for a new feature or significant update label Dec 20, 2022
@bentoi
Copy link
Contributor

bentoi commented Dec 20, 2022

I'm not fond of allowing to application to setup failure callbacks, in particular if there are alternatives such as setting up a payload writer to allow logging send failures.

I think it might also be worth investigating if we can do like many of the .NET Core APIs and use EventSource to log unexpected exceptions. See for example: https://github.com/dotnet/runtime/blob/3ce96fa45a536273f5e61600acd9868d5b34aeaf/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L2121

I don't know how they enable or use these traces to investigate issues however but it might be worth investigating?

@bernardnormier
Copy link
Member Author

I updated the proposal to use a ILogger instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal for a new feature or significant update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants