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

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.

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.

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.

@ronag PTAL

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.

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?

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.

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.

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.

Sorry if it took me a while:

#36647

Sorry to revive an ancient discussion, but what how did you intend client aborts to be handled after this change? It looks like currently the only way is to listen on close of responses and check if writableFinished is set to false.

I opened #46666 and more or less completely forgot about it 😅, since it didn't get any attention I wrote something myself, but I'm not sure if the current way should just be documented, or if the abort event should be called again. Maybe you can chime in?