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

Explicitly close connection after response body to support servers ignoring Connection: close request header #102

Closed
jmoo opened this issue Jun 23, 2018 · 5 comments · Fixed by #161

Comments

@jmoo
Copy link

jmoo commented Jun 23, 2018

Some HTTP/1.1 servers do not honor the Connection: Close header. bufferResponse will never resolve when making requests to these.

Here is an example fix:
jmoo@a9c27ec

The only problem is that \React\Promise\Stream\buffer doesn't resolve when maxLength is reached and changing it to resolve would be a BC break. We'd have to roll our own or start a discussion over on https://github.com/reactphp/promise-stream about adding a $resolveOnMaxLengthReached flag or something.

Related to #89

@clue
Copy link
Owner

clue commented Jul 30, 2018

@jmoo This is an interesting issue!

Some HTTP/1.1 servers do not honor the Connection: Close header.

The server is clearly at fault for violating HTTP specs here: https://tools.ietf.org/html/rfc7230#section-6.3

That being said, I agree that this is something we should support nonetheless. The implementation likely depends on reactphp/http-client#92 being addressed first. Once that PR is in, this should be trivial here.

@clue clue changed the title Respect content-length to prevent indefinite buffering Explicitly close connection after response body to support servers ignoring Connection: close request header Jul 30, 2018
@clue clue mentioned this issue Apr 23, 2019
@bartvanhoutte
Copy link

Seems like I'm running into this as well. Any workarounds for the moment?

@jmoo
Copy link
Author

jmoo commented Apr 28, 2020

Seems like I'm running into this as well. Any workarounds for the moment?

This was a couple of years ago but at the time I just ended up handling the buffering manually:
https://github.com/jmoo/chrome-reactphp/blob/04340bdb1f0a0371ee21dc2bc30fc12a0ec0ab1b/src/Async/Client.php#L118

@bartvanhoutte
Copy link

@jmoo just implemented something similar.

@clue
Copy link
Owner

clue commented May 5, 2020

Finally fixed via #161, please let me know if this works for you 👍

Thanks for reporting and for the possible workaround!

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

Successfully merging a pull request may close this issue.

3 participants