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

Ignore early hints for 4.9.x #7196

Closed
wants to merge 2 commits into from
Closed

Conversation

unlucku
Copy link

@unlucku unlucku commented Mar 29, 2022

Does the rfc mention if :status: has to be the first response header or not? If it does not, then a for loop to look for the status might be needed.

Alternatively this change can be put in Http2Connection.headers and we can call .get(":status:") on the headers but I think that might end up requiring more changes than it should.

@swankjesse
Copy link
Collaborator

We’re gonna revamp 1xx handling in 5.0.0. I think we get that working with our new 1xx testing stuff in MockWebServer, then backport to 4.9.x.

FYI @yschimke

@@ -65,7 +65,7 @@ class Http2Reader(
source = continuation,
headerTableSizeSetting = 4096
)

private const val EARLY_HINTS_CODE = "103"
Copy link
Collaborator

@yschimke yschimke Sep 4, 2022

Choose a reason for hiding this comment

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

This needs to be a ByteString.

Suggested change
private const val EARLY_HINTS_CODE = "103"
private val EARLY_HINTS_CODE = "103".encodeUtf8()

@yschimke
Copy link
Collaborator

yschimke commented Sep 4, 2022

I don't think the plan is to land this, but it didn't fix the issue, so probably indicates we need to be able to test it to land the eventual fix.

@yschimke
Copy link
Collaborator

Rebased onto 4.10.x and fixed the bug plus adding a test

#7443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants