request.abort() still destroys the socket on a successful request

[szmarczak]

• Version: v13.12.0
• Platform: Linux solus 5.5.11-151.current #1 SMP PREEMPT Tue Mar 24 18:06:46 UTC 2020 x86_64 GNU/Linux
• Subsystem: http

What steps will reproduce the bug?

$ node
Welcome to Node.js v13.12.0.
Type ".help" for more information.
> z = https.get('https://google.com', {agent: new https.Agent({keepAlive: true})})
ClientRequest { ... }
> z._ended
true
> z.socket.destroyed
false
> z.abort()
undefined
> z.socket.destroyed
true
> 

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

Always.

What is the expected behavior?

The socket should be still alive.

What do you see instead?

The socket is destroyed.

[szmarczak]

Please see sindresorhus/got#1154 (comment)

Got calls request.abort() when its stream is destroyed (which happens when using stream.pipeline(...)).

[sam-github]

So, if a request is in-progress, the only way with HTTP to abort the request is destroy the stream.

But if a request is ended, and someone calls abort, there isn't anything to abort.

What do you think should happen? throw an error, because it can't be aborted? a silent no-op? or what it does now, destroy the stream?

[szmarczak]

What do you think should happen? throw an error, because it can't be aborted? a silent no-op?

I think a silent no-op would be a good choice. For example, request.setTimeout(...) is a no-op if the request is fully finished.

Feel free to go with a major breaking way, throwing an error would also solve the issue. But I don't think it's a good idea to do so, because request.abort() is supposed to abort the stream, not destroy the connection.

what it does now, destroy the stream?

Yup, request.abort() destroys the socket, which can lead to another socket hung up errors if there are any requests pending.

[sam-github]

supposed to abort the stream, not destroy the connection

Those are the same thing.

Or at least, HTTP connections map 1:1 onto streams, and HTTP connections can only be aborted by destroying the underlying stream.

Also, streams only have a .destroy(), not an abort(), so "abort the stream" means "destroy the stream" (if it means anything).

In other words, abort === destroy, with the possible optimization that if a request is not in flight, it could possibly no-op or throw. But consistency is nice, rather than having APIs where the behaviour depends on the exact point in the HTTP state machine the client request is at the point of the API call, if they always do the same thing, its easier to reason about. IMO, of course.

[szmarczak]

Those are the same thing.

According to the HTTP/1.1 implementation, yes. But keep in mind example HTTP/2 introduces streams, and in that case streams !== connections.

Also, streams only have a .destroy(), not an abort(), so "abort the stream" means "destroy the stream" (if it means anything).

Agreed.

But consistency is nice, rather than having APIs where the behaviour depends on the exact point in the HTTP state machine the client request is at the point of the API call, if they always do the same thing, its easier to reason about.

It depends on how you interpret this. If you want full consistency, then you either need to always destroy the stream or throw an error. On the other hand, you just need to abort the pending request, and since the request has been completed you want to reuse the socket later (assuming you're having a keepalive agent).

[szmarczak]

What I'm asking is exactly the same #32153 but for ClientRequest.

/cc @ronag

[ronag]

This should be possible to fix. This condition needs to be improved to be less strict.

https://github.com/nodejs/node/blob/master/lib/_http_client.js#L374

[szmarczak]

Ok, I'll send a PR.

[ronag]

Hm, actually this might be more complicated than that. Might have to interact with responseKeepAlive(req) and detach the socket from the req object as well, not just the response. But then destroy gets confused and thinks the request is not yet initialized.

Might need some more significant refactoring to fix this in a nice way. Unsure.

[ronag]

@szmarczak: #33120 will probably fix this. I'm a little short on time right now. If you want to help out building some tests would be appreciated.