http: align destroy w streams #32182

ronag · 2020-03-10T17:58:17Z

This further aligns ClientRequest with streams

Streams that are prematurely destroyed without
error through .destroy() do usually not emit
an error.

Align ClientRequest with this behavior.

This changes the behavior to not error when:

abort() from req.socket has been assigned until req.res has been assigned.
destroy() until req.res has been assigned.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Streams that are prematurely destroyed without error through .destroy() do usually not emit an error. Align ClientRequest with this behavior.

ronag · 2020-03-10T19:41:28Z

I'm not 100% sure this is a good idea since I suppose people are used to destroy() and get an error if the request is incomplete (in certain situations which is probably not entirely clear for everyone). Which makes me wonder whether abort() should also error in this case. There is an inconsistency here regardless.

Though I would much prefer the behavior proposed in this PR.

nodejs-github-bot · 2020-03-10T22:44:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29710/

lpinca · 2020-03-11T20:03:31Z

lib/_http_client.js

@@ -375,9 +375,6 @@ function _destroy(req, socket, err) {
    socket.destroy(err);
  } else {
    socket.emit('free');
-    if (!req.aborted && !err) {
-      err = connResetException('socket hang up');


I don't like to remove this.

Any specific reason? If I call e.g. .destroy() on a fs stream it won't cause any error (or any other stream I can think of at the moment).

Is it backwards compat which is you concern?

It's an informative error that is being removed for no good reason. What's the problem with it? It doesn't work 100% like stream.destroy() but it is really so problematic to justify a semver-major change?

I would argue that destroying/aborting a stream is not an error and emitting an error is weird/confusing since for me that would indicate something went wrong / unexpected happened.

I do agree that the worth of the change is questionable though.

ronag · 2020-03-11T21:47:53Z

Just to give some motivation to this change. Most people (as far as I know) currently use abort() due to suboptimal behaviour for destroy() in the past. It would in my opinion be appropriate to doc deprecate abort(). However, in that case it would also be good if destroy() behaved in the same "sane" way as abort() does.

lpinca · 2020-03-12T06:50:15Z

abort() behavior was changed in #20077 and I was -0 on it. This goes one step further and completely remove the "socket hang up" error.

ronag · 2020-03-12T06:58:55Z

abort() behavior was changed in #20077 and I was -0 on it. This goes one step further and completely remove the "socket hang up" error.

Ah, I forgot about that. Got it. I'll leave this open for a few more days in case anyone else has further input and close it otherwise.

mcollina · 2020-03-12T11:00:52Z

test/parallel/test-http-client-abort-destroy.js

@@ -58,8 +58,7 @@ const assert = require('assert');
  server.listen(0, common.mustCall(() => {
    const options = { port: server.address().port };
    const req = http.get(options, common.mustNotCall());
-    req.on('error', common.mustCall((err) => {
-      assert.strictEqual(err.code, 'ECONNRESET');
+    req.on('close', common.mustCall((err) => {


I think this change is not correct.

A user is currently adding req.on('error') and req.on('response'). They are not adding req.on('close').

This confuses me a little though, abort() is what most people use (I think?) and then according to your comment above they are already broken.

I think we have two options here:

Make destroy() not force an error. (This PR currently)

Make abort() force an error.

Having the two of them behave differently is confusing.

Just to clarify, this is not about swallowing errors. This is about forcing an error.

The current behavior of http.request() implies that either 'error' is fired or 'response' is fired. Firing nothing wil definitely create a memory leak.

Then abort() has been broken for a long time, because you can call abort() and get neither 'error' nor 'response'.

This happens if abort() is called before the request has received a socket (previously through the short circuit in onSocket).

Should I change this PR to fix that instead?

Firing nothing wil definitely create a memory leak.

'close' is always fired, but I think I get your point.

I'm talking about code that has already been written and will break in subtle ways.

I'll open a separate PR to fix abort

ronag added http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 10, 2020

http: align destroy w streams

fa3b428

Streams that are prematurely destroyed without error through .destroy() do usually not emit an error. Align ClientRequest with this behavior.

ronag force-pushed the http-client-destroy-no-err branch from afea05b to fa3b428 Compare March 10, 2020 17:59

This comment has been minimized.

Sign in to view

fixup: test

6ce7477

ronag requested review from BridgeAR and lpinca March 10, 2020 19:41

ronag added the wip Issues and PRs that are still a work in progress. label Mar 10, 2020

fixup: test

7ef95e4

ronag force-pushed the http-client-destroy-no-err branch from 2144f3f to 7ef95e4 Compare March 10, 2020 22:32

ronag removed the wip Issues and PRs that are still a work in progress. label Mar 10, 2020

BridgeAR removed their request for review March 11, 2020 14:54

lpinca reviewed Mar 11, 2020

View reviewed changes

ronag requested review from mcollina and BridgeAR March 12, 2020 07:16

mcollina requested changes Mar 12, 2020

View reviewed changes

ronag closed this Mar 12, 2020

ronag mentioned this pull request Mar 12, 2020

ClientRequest .abort() and ECONNRESET #32225

Closed

trygve-lie mentioned this pull request May 11, 2023

fix: Replace .abort() with .destroy() podium-lib/client#317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: align destroy w streams #32182

http: align destroy w streams #32182

ronag commented Mar 10, 2020 •

edited

Loading

This comment has been minimized.

ronag commented Mar 10, 2020 •

edited

Loading

nodejs-github-bot commented Mar 10, 2020

lpinca Mar 11, 2020

ronag Mar 11, 2020

lpinca Mar 11, 2020

ronag Mar 11, 2020 •

edited

Loading

ronag commented Mar 11, 2020

lpinca commented Mar 12, 2020

ronag commented Mar 12, 2020

mcollina Mar 12, 2020

This comment was marked as off-topic.

ronag Mar 12, 2020 •

edited

Loading

ronag Mar 12, 2020

mcollina Mar 12, 2020

ronag Mar 12, 2020 •

edited

Loading

mcollina Mar 12, 2020

ronag Mar 12, 2020

http: align destroy w streams #32182

http: align destroy w streams #32182

Conversation

ronag commented Mar 10, 2020 • edited Loading

Checklist

This comment has been minimized.

ronag commented Mar 10, 2020 • edited Loading

nodejs-github-bot commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

ronag commented Mar 11, 2020

lpinca commented Mar 12, 2020

ronag commented Mar 12, 2020

Choose a reason for hiding this comment

This comment was marked as off-topic.

ronag Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag commented Mar 10, 2020 •

edited

Loading

ronag commented Mar 10, 2020 •

edited

Loading

ronag Mar 11, 2020 •

edited

Loading

ronag Mar 12, 2020 •

edited

Loading

ronag Mar 12, 2020 •

edited

Loading