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

Basic "100 Continue" support #95

Closed
wants to merge 1 commit into from
Closed

Conversation

nick4fake
Copy link

#94

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Thanks for filing this PR, but I'd like to see some (significant) changes to this proposal before we can get this in 👍

The intermediary 100 (Continue) response MUST NOT be sent for requests that did not include a proper Expect: 100-continue header (which we never actually send). This implies that we should only really ignore this in this case which in turn implies that we should probably also send this header automatically for certain outgoing requests. Also, the response is actually a "complete" response, which means that we actually have to buffer this to deal with response headers etc. See also #94 for more details.

@nick4fake
Copy link
Author

@clue

Also, the response is actually a "complete" response, which means that we actually have to buffer this to deal with response headers etc. See also #94 for more details.

Actually, I have copied working code from the project :)
There is a return that skips further processing

@clue
Copy link
Member

clue commented Jun 7, 2017

Perhaps an example may help, but the following is actually a valid intermediary 100 (Continue) response:

HTTP/1.1 100 Go On!\r\n
Server: ReactPHP\r\n
X-Powered-By:PHP 3\r\n
\r\n
HTTP/1.1 200 Yey!\r\n
Content-Length: 2\r\n
\r\n
OK

@nick4fake
Copy link
Author

@clue yep, you are right. I should have read RFC more accurately

@clue
Copy link
Member

clue commented Jan 18, 2018

@nick4fake Thank you for looking into this and confirming this!

I believe this has been answered, so I'm closing this for now. Please come back with more details if this persists and we can reopen this 👍

@clue clue closed this Jan 18, 2018
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.

2 participants