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

Add Trailer Headers to Ember #3602

Merged

Conversation

ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jul 25, 2020

Adds Trailer Headers to Ember - Previous to this we had been marking chunked streams not reuseable because it did not parse correctly. Specifically it was leaving bytes left in the root stream.

The old behavior stopped reading after it had parsed 0\r\n, however the termination for chunked is
0\r\n\r\n OR 0\r\nheader:keyr\r\nheader:key\r\n\r\n

Since we had previously pulled 0\r\n that means we need to parse to either an immediate \r\n, or pull into a buffer until we reach \r\n\r\n. The only thing I feel is missing is a termination condition if the bytevector for accumulation gets too large. Is there a reasonable default here?

That final \r\n\r\n or \r\n should signal the very last bytes and hence mean connections are truly ready for reuse.

Copy link
Member

@rossabaker rossabaker left a comment

The only thing I feel is missing is a termination condition if the bytevector for accumulation gets too large. Is there a reasonable default here?

You mean maxHeaderSize? There could be a maxTrailerHeaderSize, but it's such an obscure HTTP feature, I'm not sure anyone cares to make two different sizes.

rossabaker
rossabaker previously approved these changes Aug 6, 2020
Copy link
Member

@rossabaker rossabaker left a comment

👍 on scalafmt

@rossabaker rossabaker added the enhancement Feature requests and improvements label Aug 6, 2020
@rossabaker rossabaker added this to the 0.21.7 milestone Aug 7, 2020
@rossabaker rossabaker dismissed their stale review Aug 7, 2020

Not scalafmt

Copy link
Member

@rossabaker rossabaker left a comment

Actually, this one is a MiMa issue.

Copy link
Member

@rossabaker rossabaker left a comment

Everything was package private. Added problem filters and we're green.

@rossabaker rossabaker merged commit 05ba2b5 into http4s:series/0.21 Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants