res.destroy() not causing a close event on node v17

[devinivy]

Version

v17.0.0

Platform

Darwin my.host.local 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64

Subsystem

No response

What steps will reproduce the bug?

'use strict';

const Http = require('http');
const Events = require('events');

(async () => {

    const server = Http.createServer((_, res) => {

        // The log below is output on node v16 but not node v17, as 'close' is not emitted.
        res.once('close', () => console.log('close'));
        res.destroy();
    });

    server.listen(0);
    await Events.once(server, 'listening');

    try {
        const { port } = server.address();
        const req = Http.request(`http://[::1]:${port}/`).end();
        const [message] = await Events.once(req, 'response');

        message.on('data', () => null);
        await Events.once(message, 'end');
    }
    finally {
        server.close();
    }
})();

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

It reproduces consistently 👍

What is the expected behavior?

The expected behavior is for the script to log close and then output a socket hangup error:

❯ node repro.js
close
node:internal/errors:691
  const ex = new Error(msg);
             ^

Error: socket hang up
    at connResetException (node:internal/errors:691:14)
    at Socket.socketOnEnd (node:_http_client:471:23)
    at Socket.emit (node:events:402:35)
    at endReadableNT (node:internal/streams/readable:1343:12)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  code: 'ECONNRESET'
}

What do you see instead?

Instead I see the socket hangup error, but the 'close' event is not emitted, so we don't see close logged to the console.

Additional information

This was originally reproduced in the hapi test suite: https://github.com/hapijs/hapi/blob/c7cfa2e0f9c1d4ba94b4715a5a268356f68294b3/test/transmit.js#L1197-L1250

I believe this could be related to these areas within node:

node/lib/_http_outgoing.js

Lines 304 to 308 in 9125cfd

	this.destroyed = true;
	if (this.socket) {
	this.socket.destroy(error);
	} else {

node/lib/_http_server.js

Lines 841 to 847 in 9125cfd

	function emitCloseNT(self) {
	if (!self.destroyed) {
	self.destroyed = true;
	self._closed = true;
	self.emit('close');
	}
	}

Notice that destroyed is set prior to destroying the socket, and then is checked later when deciding to emit 'close'. The check is new in v17, which may be why we see this change in behavior. Potentially related to this commit, though not confirmed: f4609bd

[ronag]

Yea it should probably be if(!self._closed). Do you think you could try making a PR?

[rayw000]

Yea it should probably be if(!self._closed). Do you think you could try making a PR?

Work for me. Thanks!
I'd like to add necessary unit test for this case.