Revised request / inject abort handling#4295
Conversation
|
Why are there no CI checks active? |
|
@kanongil odd! Looking into it. (Thanks for all this work!) |
|
I can see them in the PR, perhaps there was just a delay on GH infra side? 🤔 |
|
Yes, there was an issue on GitHub's side: https://www.githubstatus.com/incidents/bdbzpz7qxmbx |
| } | ||
|
|
||
| err = err || new Boom.Boom(`Request ${event}`, { statusCode: request.route.settings.response.disconnectStatusCode }); | ||
| const error = internals.error(request, Boom.boomify(err)); |
There was a problem hiding this comment.
Is there a particular reason you dropped the use of internals.error() here? After the end of the request lifecycle in all other cases I believe hapi does ensure request.response is a response object.
The repro case in #4244 is an interesting example, since the 'response' event is used and assumes a response object exists on request.response at that point (was that a fair assumption?). In this case 'response' is emitted and request.response is instead a boom error.
Want to be clear that I am not opposed to this, just want to think through the implications together and ensure they are what we want 👍
There was a problem hiding this comment.
Thanks for the concern. I did it for consistency and to signal that it was not actually delivered. Current aborts can also set a boom object as the response, when they happen before the transmit has started:
Line 728 in ab40394
and tested as such here:
Lines 567 to 569 in 7b4d7d8
However, a case can be made that better consistency will be achieved by always making a response from it, but it seems quite wasteful and confusing to me, as there is no actual transmitted response.
WDYT?
There was a problem hiding this comment.
Taking a closer look, I agree with you that this is the most consistent behavior 👍
|
I plan to release this patch tomorrow (Friday) pending any additional review. Also, Gil is correct that this only resolves half of #4244. I think this resolves the more important and more difficult half, but it will still be worth taking another look at it to ensure we always call |
4 participants
This is a targeted PR to fix the crash issue in #4294. Unfortunately it grew quite complex due to inconsistencies around how aborts & transmission errors are reported.
The issue in #4294 was mostly covered by this existing test:
hapi/test/transmit.js
Line 528 in 18495f7
The only thing missing is to trigger the
internals.chain()loop in transmit.js, which I have done by adding aaccept-encoding: gzipheader to the request.Most of the PR comes from revising
server.inject()to handle an inconsistency, where it converts transmission errors to appear as actual server responses. This is contrary to regular http clients, which will emit / throw anerror. I'm not sure whether this can be considered a bug fix, but its plausible given that it is supposed to work similar to a regular http client, and the docs does not cover how transmission errors are signalled.FYI, I felt I had to do something, since currently 499 request abortion errors have two different response signatures depending on which stage of the requests it arrives at. One is a simple Boom error, while the other is a regular response. Ie. the
statusCodeis either found at the loggedresponse.statusCodeorresponse.output.statusCode.