http: use autoDestroy: true in incoming message

[dnlup]

Enable the default autoDestroy: true option in IncomingMessage.

Refactor _http_client and _http_server to remove any manual destroying/closing of IncomingMessage.
Refactor IncomingMessage destroy method to use the standard implementation of the stream module and move the abort logic
inside of it.

Ref:

#30625

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[ronag]

IncomingMessage overrides destroy() which kind of blocks any effort in this regards. That needs to IMO be fixed first.

[dnlup]

IncomingMessage overrides destroy() which kind of blocks any effort in this regards. That needs to IMO be fixed first.

Thanks, @ronag, for the suggestion. I'll look into it. There is an issue with destroy for sure. When using keep-alive, we don't want to destroy the socket, and that's what's happening by just setting the option to true.

[dnlup]

Sorry if this is taking a long time, but I am trying to figure out what's wrong 🙏

Removing the override of destroy() resolves the EECONRESET errors, fewer tests are failing, but request is emitting close twice, and some connections seem to hang.

[dnlup]

The changes made here are overriding the ones you made recently, @ronag . I don't know if this approach is the best.

PS: just rebased against master on the last forced push.

[BridgeAR]

@ronag PTAL

[ronag]

Please also try to remove all cases of manually managing destroyed and emitting 'close' on IncomingMessage. Having some kind of hybrid solution will make things hard to maintain IMO.

[dnlup]

Please also try to remove all cases of manually managing destroyed and emitting 'close' on IncomingMessage. Having some kind of hybrid solution will make things hard to maintain IMO.

You mean like in here? I agree, that is also the reason why the callback is not called in all cases in _destroy. I also agree that it is ugly, sorry. I wasn't sure I could change the _http_server.js parts.

Thanks for the pointers.

[dnlup]

Do we need to keep this behavior? If we do, I think it would be better to implement the destroy logic directly in IncomingMessage and not reusing the general lib/internal/streams/destroy.js implementation as I did. Overriding that implementation causes a lot of tests to fail because they are expecting an unhandled error. Also, do you have anything against adding an abort method on IncomingMessage?

[ronag]

Do we need to keep this behavior?

Yes, just call destroy and move the logic there?

If we do, I think it would be better to implement the destroy logic directly in IncomingMessage and not reusing the general lib/internal/streams/destroy.js implementation as I did.

I disagree. We should implement it as a regular destroy.

Overriding that implementation causes a lot of tests to fail because they are expecting an unhandled error.

Can still be an unhandeld error?

Also, do you have anything against adding an abort method on IncomingMessage?

Yes, destroy should be abort. We just deprecated abort on ClientRequest in favor of destroy.

[dnlup]

Yes, just call destroy and move the logic there?

Ok.

I disagree. We should implement it as a regular destroy.

I agree. The only difference I have seen with a regular destroy is that IncomingMessage emits an error only if there are listeners attached, at least when aborting.

Yes, destroy should be abort. We just deprecated abort on ClientRequest in favor of destroy.

Got it.

[ronag]

The only difference I have seen with a regular destroy is that IncomingMessage emits an error only if there are listeners attached, at least when aborting.

I can help with sorting this out. If you have any test that depends on this behavior just comment them out and we can take a look together when that's the only thing remaining.

[kanongil]

That test won't cover this case, since the response is immediately ended, and it does not read from the request.

For inspiration I made this code, which will incorrectly output REQ CLOSED on 15.5.0:

const Http = require('http');

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

    console.log('REQ', req.method, req.url)

    req.on('data', (chunk) => {

        console.log('CHUNK', chunk.length)
    });

    req.on('close', () => console.log('REQ CLOSED'));
    res.on('close', () => console.log('RES CLOSED'));
});

server.listen(() => {

    const port = server.address().port;

    const req = Http.request({ port, method: 'GET' }, (res) => {

        console.log('GOT RES', res.statusCode);
        server.close();
    });

    req.end();
});

[dnlup]

const Http = require('http');

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

    console.log('REQ', req.method, req.url)

    req.on('data', (chunk) => {

        console.log('CHUNK', chunk.length)
    });

    req.on('close', () => console.log('REQ CLOSED'));
    res.on('close', () => console.log('RES CLOSED'));
});

server.listen(() => {

    const port = server.address().port;

    const req = Http.request({ port, method: 'GET' }, (res) => {

        console.log('GOT RES', res.statusCode);
        server.close();
    });

    req.end();
});

Thank you @kanongil , now I can see the difference.

[mcollina]

We should revert this in v15 :/.

[dnlup]

I apologize

[targos]

Someone please open a revert PR against v15.x-staging

[ronag]

I apologize

Don’t worry. These things happen. Nobody’s fault. We’ll sort it out.

[ronag]

@dnlup do you want to try opening a revert PR against v15?

[ronag]

@nodejs/releasers we should probably add a note to the release notes for v16 regarding the change in behavior. Just ping me and I’ll be happy to author something.

[dnlup]

@dnlup do you want to try opening a revert PR against v15?

@ronag Yes, I am going to try. I should revert all the 7 commits in master and open a PR to v15-staging branch, right?

eb14b10...8154e47

[dnlup]

Sorry if it took me a while:

#36647