Add custom graceful shutdown handler

[afharo]

#4473 introduced a piece of logic to immediately destroy the requests during the stopping phase:

hapi/lib/core.js

Lines 509 to 513 in 22377ee

	// $lab:coverage:off$ $not:allowsStoppedReq$
	if (this.phase === 'stopping') {
	return req.destroy();
	}
	// $lab:coverage:on$ $not:allowsStoppedReq$

This caused a breaking change in my API where I have a custom onRequest handler that intercepts the request and returns a graceful 503 message explaining that the server is going down.

I tried to add a custom handler replacement in this PR, but I wanted to cover that piece of code first with the tests. I can't get my head around as to why my new tests are not hitting that code path. Can someone help me to figure it out?

Thanks!

[damusix]

LGTM, @kanongil what do you think?

[kanongil]

I don't think we should support this usage, especially since it only works on node <18.

If you want to continue doing this, you will probably need to find some hacky workaround.

[afharo]

Thanks for the review! 🧡

especially since it only works on node <18

Just FMI, why would it only work on Node <18?

My API currently runs on Node 20.15.1 with @hapi/[email protected] and the graceful responses work as expected. They break after upgrading hapi to the latest because the early req.destroy() intercepts any incoming connections before my onRequest extension kicks in.

[kanongil]

Hmm, interesting. How do you know it actually works?

The test cases we have that exercise the req.destroy() does not trigger when running node 20+, and you failed to create any as well.

I would be very much interested, if you can produce a test that runs the req.destroy() codeline on node 20+.

[afharo]

Hmm, interesting. How do you know it actually works?

The tests in my API are here: https://github.com/elastic/kibana/blob/8eceb0db4d4edb0da52564ac082859abfc6ed6e7/src/core/server/integration_tests/http/http_server.test.ts#L52-L126

You can tell that I had to change the assertions in the test when upgrading @hapi/hapi: https://github.com/elastic/kibana/pull/180986/files#diff-4df112893071b87b4b80ac7f4847851ef94d3793bb99d09697978ed907676891

The test cases we have that exercise the req.destroy() does not trigger when running node 20+, and you failed to create any as well.

I would be very much interested, if you can produce a test that runs the req.destroy() codeline on node 20+.

TBH, I'm not sure why the test suite in this repo doesn't run that code path. According to everything in the code, it should just work. Even adding hacky console logs show that the phase is stopping 🤷 .

I can try to spend a couple of extra hours today debugging to find out what happens to that code path.

[afharo]

I found the culprit!

This line closes the listener, so it won't accept more connections after calling stop().

According to Node.js docs, it changed its behavior on v19, which is probably why @kanongil recalls different behavior between v18 and 20.

I still don't get why my API behaves differently with the latest version of this library, though. But, in any case, it seems like, if we want to provide graceful rejections we might want to subscribe to this.listenerr.once('close' to apply the current callback to the .close() method, and call this.listener.close() only when all ongoing requests are completed or the shutdown timeout expires.

[kanongil]

Thank you very much for your efforts!

I have used this new knowledge to create a PR #4528, which reverts the req.destroy() behaviour.

[afharo]

Thank you! I'll close my PR then 🧡