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

Return errors for partial payload transfers #199

Merged
merged 2 commits into from Sep 25, 2017

Conversation

@kanongil
Copy link
Member

kanongil commented Sep 23, 2017

I found that the "handles responses that close early" test doesn't do what it says. It merely tests, that if someone emits close on the response, it will react as if it is aborted.

Unfortunately, node uses aborted for this, and close is never called.

I fixed the implementation to use the correct event, and replaced the test with proper ones.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Sep 23, 2017

So I found that this patch breaks the close: true option for inject() in Hapi.

Apparently subtext uses read() with a http.IncomingMessage, which does indeed have a close event. This event is emitted through shot.

I have re-added the event, and created a proper test for it.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Sep 25, 2017

If it wasn't clear, this patch fixes an actual issue with partial payload transfers, causing them to appear as successfully completed.

This should probably be back-ported to v12, since that version is likely to be used for quite some time.

@geek geek self-requested a review Sep 25, 2017
@geek geek self-assigned this Sep 25, 2017
@geek geek added the bug label Sep 25, 2017
@geek geek added this to the 13.0.2 milestone Sep 25, 2017
@geek

This comment has been minimized.

Copy link
Member

geek commented Sep 25, 2017

@kanongil thanks for the fix. I'll backport to v12

@geek geek merged commit 080ef88 into hapijs:master Sep 25, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.