Crashes upon request on Node.js v15.5.0

[vsnthdev]

Support plan

• is this issue currently blocking your project? (yes/no): yes
• is this issue affecting a production system? (yes/no): no

Context

• node version: 15.5.0
• module version with issue: 20.0.3
• last module version without issue: N/A
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi application
• any other relevant information: N/A

What are you trying to achieve or the steps to reproduce?

Trying to start a Hapi.js server on node v15.5.0, using the following code from official docs 👇

import Hapi from '@hapi/hapi';

const server = Hapi.server({
    host: 'localhost',
    port: 8080
})

await server.start()
console.log(`Listening at ` + server.info.uri)

What was the result you got?

When a request is sent, the server crashes with the following message 👇

/Users/vasanth/Desktop/hapi-test/node_modules/@hapi/hapi/lib/request.js:497
            if (this.response.statusCode === 500 &&
                              ^

TypeError: Cannot read property 'statusCode' of null
    at Request._finalize (/Users/vasanth/Desktop/hapi-test/node_modules/@hapi/hapi/lib/request.js:497:31)
    at Request._reply (/Users/vasanth/Desktop/hapi-test/node_modules/@hapi/hapi/lib/request.js:434:18)
    at Request._execute (/Users/vasanth/Desktop/hapi-test/node_modules/@hapi/hapi/lib/request.js:280:14)
    at processTicksAndRejections (node:internal/process/task_queues:93:5)

What result did you expect?

The server to function normally and respond to requsets.

[vsnthdev]

Upon further inspection, I find that the server only crashes on Node.js v15.5.0 and perfectly on Node.js v15.4.0. Here is some additional information useful for more investigation on the issue 👇

Operating System: macOS Big Sur v11.1
Package Manager: Yarn v1.22.10
NPM Version: v7.3.0

[kanongil]

I can confirm that the hapi test suite fails on node v15.5.0 (also on Linux), with similar issues.

The only change that seems relevant, is the refactor in: nodejs/node#33035.

[cjihrig]

@kanongil beat me to it 😄. It's really annoying that all of these autoDestroy changes continue to break hapi.

[vsnthdev]

I really didn't believe it initially, but then the only change I made was to update to node v15.5.0, so a quick roll back of node revealed the issue.

[devinivy]

Thanks to Gil's correspondence on nodejs/node#33035 helping to illustrate the issue, this change in node is being reverted: nodejs/node#36647. I will try to keep an eye out for if node tries to re-land the original autoDestroy change in a non-breaking way. It does look like node is also adding a test that would have caught this, which is great: nodejs/node#36645. Once the reversion to node v15 is landed and published, I will close this issue after confirming hapi's test suite passes.

[mcollina]

Hi Folks, it would be great if Hapi to get into https://github.com/nodejs/citgm so that we can automatically spot regressions.

(FWIW this change is likely to make it in v16)

[kanongil]

@mcollina You should probably address your concerns towards the CITGM project instead given that there is a 5 year old open issue (nodejs/citgm#8) and a PR (nodejs/citgm#436).

[matthieusieben]

Having encountered this issue, and looked into its cause, I think you might want to re-consider flagging this as a bug.

The problem occurs on _special routes, when internals.drain is executed in the early stages of the life cycle. internals.drain will consume the req stream, which (in node 15.5) will cause both end and close events to be triggered directly (close being triggered in a "next tick").

This close event is then captured by internals.event (in request.js) that fails to detect that the lifecycle is "still ongoing", and that a close on req should not be considered as a failure at that point.

Because internals.drain will cause close to be triggered in the tick directly after end, this event occurs while the _lifecycle is still being processed.

In my humble opinion, request.js's internals.event should account for early close events, by doing something like (i didn't test but you get the idea):

    if (!request) {
        return;
    }

    // autoDestroy is enabled on the `req` stream, and the `req` stream was drained, this is basically the same as the "end" event
    // maybe request.raw.req._readableState.ended should be used instead?
    if (event === 'close' &&
        !request._isPayloadPending) {

        return;
    }

    request._isPayloadPending = false;

[devinivy]

@matthieusieben thanks for taking a close look, I see what you mean. Do you expect that this case can be triggered on node v12/v14? If there's a way to reproduce a bug then I will open a new issue for it, since this issue is now effectively tracking whether node v15 is patched. Either way I will spend some time with the info you provided above, and re-trace your steps in debugging.

[kanongil]

This is fundamentally a node issue. Given that hey have decided to release v15.5.1 with security fixes, without reverting the behavior, puts us in an awkward situation where there is no node v15 versions that are safe to use with hapi. This is somewhat OK, since I believe the official stance that only LTS releases are considered supported.

That being said, it is simple to fix in hapi in a way that will work with all node versions. All that is required, is to change the 'close' listener to listen to res instead of req. However, this expose an issue in shot, where the close option only closes the req, and not the res, which will make a hapi test fail incorrectly.

[matthieusieben]

I know it's ugly but how about doing this ?

if (process.version.startsWith('v15.')) {
  this.raw.res.on('close', internals.event.bind(this.raw.res, this._eventContext, 'close'));
} else {
  this.raw.req.on('close', internals.event.bind(this.raw.req, this._eventContext, 'close'));
}

[mcollina]

This is fundamentally a node issue. Given that hey have decided to release v15.5.1 with security fixes, without reverting the behavior, puts us in an awkward situation where there is no node v15 versions that are safe to use with hapi. This is somewhat OK, since I believe the official stance that only LTS releases are considered supported.

The security releases where planned and prepared before this issue was opened. Unfortunately there was no window of opportunity to ship a v15 release before the security fixes due to the xmas break (and we couldn't delay the security releases, in case you are wondering). Note that the revert in v15 has already landed and it will be part of the next v15 release: nodejs/node#36647.

The original change will still be present in v16 as it's deemed correct. This leaves probably some time for you to figure how to fix it on the Hapi side.

[devinivy]

Thank you @mcollina 👍 It's especially useful to know that in v16 the original change will be present. For those following along: I will open a separate issue to address this. It sounds like we already have a pretty good understanding of what needs to be done through all the investigation that has been done for this issue.

[kanongil]

I know it's ugly but how about doing this ?

if (process.version.startsWith('v15.')) {
  this.raw.res.on('close', internals.event.bind(this.raw.res, this._eventContext, 'close'));
} else {
  this.raw.req.on('close', internals.event.bind(this.raw.req, this._eventContext, 'close'));
}

I don't expect that there is a need to special case this. Using res is currently just as correct, and with v16 it will be THE correct way.

[devinivy]

I think we can close this one out :)

Node v15.6.0 reverted the change, shot now supports the res close event in hapijs/shot#139, and there's a PR to future-proof hapi for node v16 in #4225.