emit res close event

[cjihrig]

This commit updates the response logic to emit the 'close' event if the request is closed.

Refs: hapijs/hapi#4208

I tested these changes in hapi's master branch (combined with the change mentioned here) on Node 15.4.0, 15.5.1 (the currently broken release), 14.15.0, and 12.18.4. 🤷

[kanongil]

This seems to do the trick but you could make it simpler, and avoid tracking the state, if you just condition the req.once('close', …) on req._shot.simulate.close.

Also, a test would be nice to ensure we don't regress.

[cjihrig]

This seems to do the trick but you could make it simpler, and avoid tracking the state, if you just condition the req.once('close', …) on req._shot.simulate.close.

I was hoping to detect any naturally occurring 'close' events too, and not just the simulated ones.

I'll look into adding a test.

[kanongil]

I am inclined to perform fixes on this with surgical precision to avoid any unexpected surprises. The implementation is quite brittle.

[devinivy]

It's a hard call for me. On one hand, this fix by Colin clearly enforces that when the request is closed that the response will follow, and there is value in ensuring that is always taken care of no matter the circumstance, since this is definitely what one can typically expect to observe with a real node http server. On the other hand, it is a little bit of a blunt instrument and doesn't a. give much intuition for why this is correct behavior or b. provide room for additional nuance or edge-cases if we find this isn't 100% correct. For example, I am not convinced that the nextTick() between the request close and response close occurs in practice when the underlying socket closes prematurely, but the nextTick() may be necessary for this one-size-fits-all solution (perhaps Colin will be able to confirm/deny that suspicion).

My understanding is that what really happens in the node http server is that the underlying socket closing (a la simulate.close) triggers 1. the request to be effectively destroyed (this is managed by the http Server) and 2. the response to emit a close if it hasn't already finished writing (this is managed by the HttpResponse itself). The only other way the response will close is if it finishes writing. I think it would be ideal if our solution reinforced this by either:

• using the current solution and documenting what we know about this including any caveats by adding code comments,
• using Gil's solution,
• or using a similar mechanism to node, actually making the source of the simulated 'close' event the mock socket and leaning more on node's internal handling of this situation to bubble it up to the response.

[kanongil]

My point is to keep the fix simple, since it is a whole mess to untangle otherwise. This is further complicated by the fact that the exact node behavior can change between node releases.