Skip to content

HTTP2: Send remaining data and trailers together #13914

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 8 commits into from
Sep 20, 2019

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Sep 11, 2019

Fixes #11270

The test checks that the data is correct, but it doesn't verify that data and trailers were flushed together. What is the best way to check this using the current HTTP/2 test pattern?

@jkotalik
Copy link
Contributor

The test checks that the data is correct, but it doesn't verify that data and trailers were flushed together. What is the best way to check this using the current HTTP/2 test pattern?

I can't think of a great way here. You would need to be able to hang somewhere between WriteData and writing the response trailers to verify that the data wasn't flushed.

@Tratcher
Copy link
Member

No, we don't have a great way to check for the absence of a gap between frames. Checking for the presence of a gap would be easier.

@JamesNK
Copy link
Member Author

JamesNK commented Sep 11, 2019

I've added a check for flow control and a test - ResponseTrailers_WithLargeUnflushedData_DataExceedsFlowControlAvailableAndNotSentWithTrailers - but the test hangs while reading the large body. 1 byte remains in the data frame but the server is not sending it.

I expected ExpectAsync to unblock flow control and allow the server to send more data. Is that not the case?

@Tratcher
Copy link
Member

Flow control is unblocked by sending window updates frames (manually), not by calling ExpectAsync.

@JamesNK
Copy link
Member Author

JamesNK commented Sep 11, 2019

Thanks. Test now verifies flow control is respected.

@Tratcher
Copy link
Member

Much better. I assume it shows up as expected in a wireshark trace?

@JamesNK
Copy link
Member Author

JamesNK commented Sep 11, 2019

app.Run(context =>
{
    context.Response.BodyWriter.GetMemory(1000);
    context.Response.BodyWriter.Advance(1000);
    context.Response.AppendTrailer("trailer", "trailer2");

    return Task.CompletedTask;
});

Wireshark of headers, data and headers (trailers) sent in a single packet:

image

@JamesNK
Copy link
Member Author

JamesNK commented Sep 20, 2019

@Tratcher Ping. Could you re-review and either approve or give feedback of issues to resolve.

@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Sep 20, 2019
@JamesNK JamesNK merged commit 54ecdae into master Sep 20, 2019
@JamesNK JamesNK deleted the jamesnk/http2-trailers-with-data branch September 20, 2019 20:48
@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
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

HTTP2: Send trailers with data
6 participants