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

[4.x] 6121 100 continue request reset fix #6251

Merged
merged 2 commits into from
Feb 19, 2023

Conversation

danielkec
Copy link
Contributor

Fixes #6121

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 18, 2023
@danielkec danielkec requested a review from klustria February 18, 2023 18:50
@danielkec danielkec self-assigned this Feb 18, 2023
@danielkec danielkec changed the title 6121 100 continue request reset fix [4.x] 6121 100 continue request reset fix Feb 18, 2023
@klustria
Copy link
Member

klustria commented Feb 19, 2023

Change looks good. However, just one question which I also posted in #6121

I was encountering issue reading the http status after a continue-100 response and found that the main reason for this is because of this: https://github.com/helidon-io/helidon/blob/main/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java#L58 where there is extra /r/n at the end of Continue 100 response. Prior to the expect-continue-100 recent change, there is only 1 /r/n but in the recent code there are 2. Is this a requirement to have 2x CRLF?

I can of course skip/ignore the remaining lines, but just wondering if this is a requirement.

Copy link
Member

@klustria klustria left a comment

Choose a reason for hiding this comment

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

I will approve this anyway because I have tested the change to be working fine after applying it in my local branch. Also, I can deal with the extra CRLF, but would just like to verify if that is really needed for a 100-Continue response. Thanks for the quick turnaround, even on your weekend.

@danielkec
Copy link
Contributor Author

danielkec commented Feb 19, 2023

Hi @klustria afaik double CRLF is mandatory, see RFC 2616

Response      = Status-Line *(( general-header | response-header | entity-header ) CRLF)  
                       CRLF        (* <---- Second *)
                       [ message-body ]          

Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF (* <----- First *)

@danielkec danielkec merged commit 54d47b9 into helidon-io:main Feb 19, 2023
@klustria
Copy link
Member

Thanks @danielkec!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants