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

Fix some corner case client errors #3971

Merged
merged 3 commits into from Sep 14, 2019

Conversation

@kanongil
Copy link
Member

commented Aug 20, 2019

It turns out that the trigger in #3969 does indeed contain a bug. Namely that a request, that is aborted during the request lifecycle, can cause a timeout, and log a 503 response even though nothing was ever sent down the wire.

Further investigation found that this was caused by the stream draining code in the default handler, which never completes if the stream is aborted. This in turn means that the _lifecycle() call in request._execute() never returns, and thus skips the call to _reply() that would clear the timeout timer.

The other part of the patch relates to the clientError handling. Here I found a case where a client can receive a 400 Bad Request response, while the server logs it as a regular response.
This is because, contrary to the node docs, the clientError event can be triggered while processing an request on the socket, which hapi doesn't expect.

I have updated the handler, to send a 400 Bad Request response through any outstanding requests instead of writing it directly to the socket, which seems to fix the issue.

kanongil added 3 commits Aug 20, 2019
This ensures that any active request tied to the socked is logged correctly
@hueniverse hueniverse self-assigned this Sep 14, 2019
@hueniverse hueniverse added the bug label Sep 14, 2019
@hueniverse hueniverse added this to the 18.4.0 milestone Sep 14, 2019
@hueniverse hueniverse merged commit 9279db5 into hapijs:master Sep 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hueniverse added a commit that referenced this pull request Sep 14, 2019
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.