[x] http: client destroy stream #29192

ronag · 2019-08-18T12:55:57Z

This PR tries to re-use the destroy logic from streams. One small step towards making ClientRequest more stream-like.

~~Based on #28683. Since it's close to being merged I don't want to complicate it further.~~

Unlike #28683. Doesn't currently swallow errors. But will only emit one error after deestroy().

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

ronag · 2019-08-18T13:05:55Z

@mcollina: A little guidance here? Merge #28683 first and then have this as a continuation? Or merge this into #28683 and do another round of reviews?

lib/_http_client.js

ronag · 2019-09-18T23:12:38Z

@mcollina: Any further changes?

@Trott: This needs more reviews?

mcollina

LGTM

lib/_http_client.js

mcollina · 2019-09-20T09:10:27Z

cc @nodejs/tsc

Trott · 2019-09-22T22:35:43Z

(Needs a rebase.)

ronag · 2019-09-23T06:07:42Z

@Trott rebased

nodejs-github-bot · 2019-09-24T02:54:05Z

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

mcollina

Can you add some more tests about this behavior change?

test/parallel/test-http-client-set-timeout.js

test/parallel/test-http-client-close-event.js

lib/_http_client.js

ronag · 2019-09-24T06:36:55Z

Can you add some more tests about this behavior change?

Not sure what that would test. I'll think about it and give it a try.

mcollina · 2019-09-24T07:05:52Z

lib/_http_client.js

+  } else if (this[kWritableState].socketPending) {
+    // In the event that we don't have a socket, we will pop out of
+    // the request queue through handling in onSocket.
+    this[kWritableState].destroyCallback = (er) => cb(er || err);


I think it's better to wait for the 'socket' event to be emitted instead of adding one more state parameter: https://nodejs.org/api/http.html#http_event_socket.

That won't work, the socket event is not always emitted. See the error case in oncreate, i.e. close to where your other comment is.

The error from oncreate needs to be properly forwarded to the _destroy callback.

From a user perspective, I think we should be swallowing that error if destroy() was called before hand.

Do we test that calling destroy() before we have a socket actually calls socket.destroy()?

This can be further refactored if/once #29656 is merged. But it basically does the same thing.

I think we should be swallowing that error if destroy() was called before hand.

This confuses me. Isn't it a bit inconsistent with the discussion in #29197? @lpinca might have input?

Do we test that calling destroy() before we have a socket actually calls socket.destroy()?

Not sure, I will check.

Do we test that calling destroy() before we have a socket actually calls socket.destroy()?

Test added

The possibility of deadlocks/having a callback not called in the destroy process makes me nervous. The current logic is significantly simpler, even if less correct, and I feel this might bite us in the future.

The possibility of deadlocks/having a callback not called in the destroy process makes me nervous.

That's the way Node works? This is not the only case? What can we do to make you less nervous? More tests?

The current logic is significantly simpler, even if less correct, and I feel this might bite us in the future.

I strongly disagree with this. ~~In my opinion this kind of mentality will stagnate the project.~~ Shouldn't we strive after correctness and consistency?

We have similar scenarios in a lot of different places and I have pending PR's (e.g. #29656) with efforts on this.

If this (swallow error) is the way we go then I'd like to ask #29197 to be reconsidered (to "no error after destroy") in order to achieve some form of consistency. Otherwise, we will never have "correct" code even in the future.

Shouldn't we strive after correctness and consistency?

No. We should strive to not break current users, and then consistency. Correctness it's extremely hard to measure because there are no specifications for any of this.

We should strive to not break current users

How does this break current users?

ronag · 2019-09-24T18:28:32Z

I've tried adding more test for what make sense. However, most of these changes are refactoring in nature as well as re and proper use of existing framework.

ronag · 2019-11-24T10:04:47Z

This PR can probably be easier implemented with #29656

ronag · 2019-12-15T10:34:06Z

This is blocked by #29656

ronag · 2019-12-29T19:49:17Z

There are several pending PR's (.e.g #30623, #29656) that affects this which means this is not ready. I will close this for now and re-open when it becomes relevant again.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 18, 2019

ronag force-pushed the http-client-destroy-stream branch 2 times, most recently from 595b846 to 4a82370 Compare August 18, 2019 13:03

ronag force-pushed the http-client-destroy-stream branch 21 times, most recently from 9223798 to f904ea8 Compare August 18, 2019 14:14

mscdex reviewed Aug 18, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

ronag force-pushed the http-client-destroy-stream branch 4 times, most recently from 5f4f9d6 to 19425c2 Compare August 18, 2019 15:13

wa-Nadoo reviewed Aug 25, 2019

View reviewed changes

lib/_http_client.js Show resolved Hide resolved

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 20, 2019

mcollina approved these changes Sep 20, 2019

View reviewed changes

lib/_http_client.js Show resolved Hide resolved

ronag force-pushed the http-client-destroy-stream branch from 822a66e to f4bf6d2 Compare September 23, 2019 06:07

mcollina reviewed Sep 24, 2019

View reviewed changes

test/parallel/test-http-client-set-timeout.js Show resolved Hide resolved

test/parallel/test-http-client-close-event.js Show resolved Hide resolved

lib/_http_client.js Show resolved Hide resolved

mcollina requested changes Sep 24, 2019

View reviewed changes

ronag requested review from jasnell, mscdex and lpinca September 24, 2019 18:02

ronag force-pushed the http-client-destroy-stream branch from 22e5830 to 66e6272 Compare September 24, 2019 18:10

ronag force-pushed the http-client-destroy-stream branch 3 times, most recently from 1016ea1 to 09d15d5 Compare September 24, 2019 19:28

ronag mentioned this pull request Sep 24, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

ronag force-pushed the http-client-destroy-stream branch from 09d15d5 to 8441fc5 Compare November 24, 2019 10:02

http: clientRequest.abort is destroy

1483ef1

ronag force-pushed the http-client-destroy-stream branch from 8441fc5 to 1483ef1 Compare November 24, 2019 10:03

ronag closed this Dec 29, 2019

ronag changed the title ~~http: client destroy stream~~ [x] http: client destroy stream Dec 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[x] http: client destroy stream #29192

[x] http: client destroy stream #29192

ronag commented Aug 18, 2019 •

edited

ronag commented Aug 18, 2019 •

edited

ronag commented Sep 18, 2019

mcollina left a comment

mcollina commented Sep 20, 2019

Trott commented Sep 22, 2019

ronag commented Sep 23, 2019

nodejs-github-bot commented Sep 24, 2019

mcollina left a comment

ronag commented Sep 24, 2019

mcollina Sep 24, 2019

ronag Sep 24, 2019 •

edited

mcollina Sep 24, 2019

ronag Sep 24, 2019

ronag Sep 24, 2019 •

edited

ronag Sep 24, 2019

mcollina Sep 25, 2019

ronag Sep 25, 2019 •

edited

mcollina Sep 27, 2019

ronag Sep 27, 2019

ronag commented Sep 24, 2019 •

edited

ronag commented Nov 24, 2019

ronag commented Dec 15, 2019

ronag commented Dec 29, 2019

[x] http: client destroy stream #29192

[x] http: client destroy stream #29192

Conversation

ronag commented Aug 18, 2019 • edited

Checklist

ronag commented Aug 18, 2019 • edited

ronag commented Sep 18, 2019

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 20, 2019

Trott commented Sep 22, 2019

ronag commented Sep 23, 2019

nodejs-github-bot commented Sep 24, 2019

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Sep 24, 2019

mcollina Sep 24, 2019

Choose a reason for hiding this comment

ronag Sep 24, 2019 • edited

Choose a reason for hiding this comment

mcollina Sep 24, 2019

Choose a reason for hiding this comment

ronag Sep 24, 2019

Choose a reason for hiding this comment

ronag Sep 24, 2019 • edited

Choose a reason for hiding this comment

ronag Sep 24, 2019

Choose a reason for hiding this comment

mcollina Sep 25, 2019

Choose a reason for hiding this comment

ronag Sep 25, 2019 • edited

Choose a reason for hiding this comment

mcollina Sep 27, 2019

Choose a reason for hiding this comment

ronag Sep 27, 2019

Choose a reason for hiding this comment

ronag commented Sep 24, 2019 • edited

ronag commented Nov 24, 2019

ronag commented Dec 15, 2019

ronag commented Dec 29, 2019

ronag commented Aug 18, 2019 •

edited

ronag commented Aug 18, 2019 •

edited

ronag Sep 24, 2019 •

edited

ronag Sep 24, 2019 •

edited

ronag Sep 25, 2019 •

edited

ronag commented Sep 24, 2019 •

edited