Revised request / inject abort handling

[kanongil]

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

it('does not crash when request is aborted', async () => {

The only thing missing is to trigger the internals.chain() loop in transmit.js, which I have done by adding a accept-encoding: gzip header 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 an error. 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 statusCode is either found at the logged response.statusCode or response.output.statusCode.

[kanongil]

Why are there no CI checks active?

[devinivy]

@kanongil odd! Looking into it. (Thanks for all this work!)

[Nargonath]

I can see them in the PR, perhaps there was just a delay on GH infra side? 🤔

[cjihrig]

Yes, there was an issue on GitHub's side: https://www.githubstatus.com/incidents/bdbzpz7qxmbx

[devinivy]

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 👍

[kanongil]

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:

hapi/lib/request.js

Line 728 in ab40394

request._setResponse(new Boom.Boom('Request aborted', { statusCode: request.route.settings.response.disconnectStatusCode }));

and tested as such here:

hapi/test/transmit.js

Lines 567 to 569 in 7b4d7d8

	const [request] = await log;
	expect(request.response.isBoom).to.be.true();
	expect(request.response.output.statusCode).to.equal(499);

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?

[devinivy]

Taking a closer look, I agree with you that this is the most consistent behavior 👍

[devinivy]

I had one open question, but I poked at this quite a bit and I feel it is a solid patch! I am pleased to see that this addresses #4244 without running into the awkward coverage issue I ran into, which I think is a good sign.

[Nargonath]

I'm not that familiar with that part of the framework but it LGTM. Well done @kanongil 💪 .

[kanongil]

I am pleased to see that this addresses #4244 without running into the awkward coverage issue I ran into, which I think is a good sign.

I'm not sure this PR fully addresses #4244. I have made no effort towards always calling res.end().

[Nargonath]

I've linked the issue #4294 to this MR for it to be closed automatically but feel free to remove it @kanongil in case it was a mistake.

[devinivy]

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 res.end() during aborts. Thanks, all!