Skip to content

[release/6.0] HTTP/3: Use new QuicStream.ReadsCompleted property in transport #35476

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[release/6.0] HTTP/3: Use new QuicStream.ReadsCompleted property in transport #35476

wants to merge 1 commit into from

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Aug 18, 2021 •
edited by Tratcher
Loading

Fixes #35399 Made this change in this branch first because main hasn't gotten latest runtime bits yet.

Tratcher
Tratcher approved these changes Aug 19, 2021
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Does the Http/3 layer need any adjustments, or should that just work?

@JamesNK
Copy link
Member Author

JamesNK commented Aug 19, 2021

HTTP/3 layer will just work. It is already using the ReadResult.IsCompleted flag here:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs

Lines 428 to 434 in 6350af9

while (Http3FrameReader.TryReadFrame(ref readableBuffer, _incomingFrame, out var framePayload))
{
Log.Http3FrameReceived(ConnectionId, _streamIdFeature.StreamId, _incomingFrame);
consumed = examined = framePayload.End;
await ProcessHttp3Stream(application, framePayload, result.IsCompleted && readableBuffer.IsEmpty);
}

adityamandaleeka
adityamandaleeka reviewed Aug 19, 2021
View reviewed changes
src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
}
finally
{
// If Shutdown() has already bee called, assume that was the reason ProcessReceives() exited.
Copy link
Member

@adityamandaleeka adityamandaleeka Aug 19, 2021

Choose a reason for hiding this comment

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

not new code, but typo: "bee" -> "been"

@JamesNK
Copy link
Member Author

JamesNK commented Aug 19, 2021

/backport to main

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2021

Started backporting to main: https://github.com/dotnet/aspnetcore/actions/runs/1145307320

@JamesNK
Copy link
Member Author

JamesNK commented Aug 19, 2021

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2021

Started backporting to release/6.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/1145308311

This was referenced Aug 19, 2021
Merged
Merged
@JamesNK
Copy link
Member Author

JamesNK commented Aug 19, 2021

Closing this PR and have changes be merged from RC1 into back into 6.0 automatically.

@JamesNK JamesNK closed this Aug 19, 2021
@dougbu dougbu deleted the jamesnk/http3-readscompleted branch August 21, 2021 22:32
@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

Reviewers

@adityamandaleeka adityamandaleeka adityamandaleeka approved these changes

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@halter73 halter73 Awaiting requested review from halter73

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JamesNK @adityamandaleeka @Tratcher @amcasey