Skip to content

HTTP/3: Add IStreamAbortFeature #34409

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 7 commits into from
Aug 3, 2021
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 16, 2021

Fixes #31970
Fixes #33575

HTTP/3 streams support aborting individual sides of a connection stream. Feature is so HTTP layer can abort reading/writing in transport layer.

Args:

  • errorCode is sent to the peer. long is used because it is variable length.
  • Exception is used to complete transport pipes.

Example usage:

The client is uploading data to the server in an HTTP/3 request. The server completes RequestDelegate (i.e. user app code) without reading all of the data. HTTP/3 layer in the server will gracefully finish writing the response and abort reading the request. The abort tells the client it can stop uploading.

namespace Microsoft.AspNetCore.Connections.Features
{
    /// <summary>
    /// Supports aborting individual sides of a connection stream.
    /// </summary>
    public interface IStreamAbortFeature
    {
        /// <summary>
        /// Abort the read side of the connection stream.
        /// </summary>
        /// <param name="errorCode">The error code to send with the abort.</param>
        /// <param name="abortReason">An optional <see cref="ConnectionAbortedException"/> describing the reason to abort the read side of the connection stream.</param>
        void AbortRead(long errorCode, ConnectionAbortedException abortReason);

        /// <summary>
        /// Abort the write side of the connection stream.
        /// </summary>
        /// <param name="errorCode">The error code to send with the abort.</param>
        /// <param name="abortReason">An optional <see cref="ConnectionAbortedException"/> describing the reason to abort the write side of the connection stream.</param>
        void AbortWrite(long errorCode, ConnectionAbortedException abortReason);
    }
}

@JamesNK JamesNK added area-runtime api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 16, 2021
@JamesNK JamesNK force-pushed the jamesnk/http3-streamabortdirection branch from bcfc1b4 to 6c837c9 Compare July 27, 2021 04:26
Comment on lines 11 to 23
/// <summary>
/// Abort the read side of the connection stream.
/// </summary>
/// <param name="errorCode">The error code to send with the abort.</param>
/// <param name="abortReason">An optional <see cref="ConnectionAbortedException"/> describing the reason to abort the read side of the connection stream.</param>
void AbortRead(long errorCode, ConnectionAbortedException abortReason);

/// <summary>
/// Abort the write side of the connection stream.
/// </summary>
/// <param name="errorCode">The error code to send with the abort.</param>
/// <param name="abortReason">An optional <see cref="ConnectionAbortedException"/> describing the reason to abort the write side of the connection stream.</param>
void AbortWrite(long errorCode, ConnectionAbortedException abortReason);
Copy link
Member Author

@JamesNK JamesNK Jul 27, 2021

Choose a reason for hiding this comment

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

In HTTP/3 the read and write sides of the stream can be aborted independently. An error code is sent with these aborts.

I followed the pattern of ConnectionContext.Abort(ConnectionAbortedException abortReason) which uses the exception to pass in a reason. The reason isn't sent to the peer in HTTP/3 and is only used by logging. It can be removed if we don't think logging a reason in the transport adds value.

Copy link
Member Author

@JamesNK JamesNK Jul 27, 2021

Choose a reason for hiding this comment

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

Do thumbs up mean keep the exception? 😄

Update: Ok, I see the point of passing exception in now. It is used with Input.Complete and Output.Complete.

@JamesNK
Copy link
Member Author

JamesNK commented Jul 27, 2021

@halter73 @Tratcher

{
if (_stream.CanRead)
{
_log.StreamAbortRead(this, abortReason.Message);
Copy link
Member

Choose a reason for hiding this comment

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

If there's an active reader, can we throw this ConnectionAbortedException from the current read similar to what we do with SocketConnection?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a test and see what happens.

Assert.Contains(LogMessages, m => m.Message.Contains("the application completed without reading the entire request body."));
Assert.Equal("The application completed without reading the entire request body.", requestStream.AbortReadException.Message);
Copy link
Member

Choose a reason for hiding this comment

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

Does this effectively get logged twice now? I'm okay with it if it's different categories and they're both debug level. Just checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's logged once at the HTTP layer and once and the transport layer.

@JamesNK JamesNK force-pushed the jamesnk/http3-streamabortdirection branch from 62bbcbb to c115fc8 Compare July 28, 2021 05:36
@JamesNK JamesNK added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 29, 2021
@ghost
Copy link

ghost commented Jul 29, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@adityamandaleeka adityamandaleeka added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 2, 2021
@JamesNK JamesNK force-pushed the jamesnk/http3-streamabortdirection branch from c115fc8 to d348453 Compare August 2, 2021 21:23
@JamesNK JamesNK force-pushed the jamesnk/http3-streamabortdirection branch from d348453 to ed7e709 Compare August 2, 2021 22:02
@JamesNK JamesNK enabled auto-merge (squash) August 3, 2021 00:45
@JamesNK JamesNK merged commit f5e411e into main Aug 3, 2021
@JamesNK JamesNK deleted the jamesnk/http3-streamabortdirection branch August 3, 2021 12:49
@ghost ghost added this to the 6.0-rc1 milestone Aug 3, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/3: Support aborting one direction of a stream HTTP/3: Handle server completing request before reading all the request body
5 participants