Undocumented breaking change on v16.0.0

[mmarchini]

• Version: v16.0.0
• Platform: OS X
• Subsystem: http

What steps will reproduce the bug?

1. Save the script below as index.js and run it.

'use strict'
const http = require('http');

const requestListener = function (req, res) {
  req.on('data', () => {});

  req.once('end', () => {
    console.log(1);
  });

  req.once('close', () => {
    console.log(2);
  });
}

const server = http.createServer(requestListener);
server.listen(8080);

2. Run curl localhost:8080

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

Prior to v16, the server would print the output below and hang.

$ node index.js
1

What do you see instead?

On v16, the server prints the output below and hang.

$ node index.js
1
2

On both cases curl doesn't exit while waiting for a response. Note that 2 is being called because close is being emitted even though the connection is not closed yet. I'm not sure if that is intended behavior or not, but I couldn't find anything in the changelog suggesting this was an intentional change. Furthermore, it's a breaking change and it should be documented as such.

Additional information

The example above is an overly simplification of a situation I've stumbled upon while investigating failing restify tests on Node.js v16. The failing test in question is this, and the failure happens because restify expects close to only be called when the connection closes.

[mscdex]

I would guess that the 'close' event is specific to the request stream and not the underlying socket.

[mmarchini]

From http.IncomingMessage close documentation:

Event: 'close'

Added in: v0.4.2

Indicates that the underlying connection was closed.

So the (documented) intention is not that the stream was closed, but the underlying [socket] connection. That's still our documentation on v16, which makes me think this might've been an unintentional change. cc @nodejs/tsc @nodejs/http

[ronag]

The current behavior is correct per se and totally intentional to be compliant with streams. I would rather update the documentation which is wrong IMO.

The old behavior basically means that using it in a streams api is undefined behavior from the user’s perspective.

[mmarchini]

It needs to be both documented as a breaking change in our changelogs and the documentation needs to be updated then. Right now this is a breaking change that is not documented anywhere. In which PR was it introduced?

[ghermeto]

I just added a comment before here thinking this was a breaking change on a minor... 😳

Don't mind me...

[targos]

If we don't find the PR which introduced the change, it might land soon by mistake on v14.x.

[ronag]

I’m pretty sure the Pr was semver. Can’t find it at the moment. Will look a bit more tonight.

[ronag]

Wait. This is on IncomingMessage which is a Readable. I’m very surprised if the behavior is different on node 14.

[ronag]

It might be doe autoDestroy true change we did.

[targos]

It might be doe autoDestroy true change we did.

Do you mean #33035 ?
It was reverted in #36647 before the release of v16.0.0

[ronag]

It was only reverted on v15 and then made it into v16 as a semver major. I think the fix here is docs and maybe missing in change log.

[targos]

Oh I see, it was actually landed initially as semver-minor.
Our release tooling is not capable of detecting that a change was retroactively marked major. I agree the fix is to update the docs (add a "changes" entry somewhere)

[targos]

Our release tooling is not capable of detecting that a change was retroactively marked major.

It's not only about major changes. If something lands on master, is released on some version, and is then reverted only on a release branch, the next major is not going to have this change in its changelog.