Skip to content

[x] http: client destroy stream - #29192

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-client-destroy-stream
Closed

[x] http: client destroy stream#29192
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-client-destroy-stream

Conversation

@ronag

@ronagronag commented Aug 18, 2019

Copy link
Copy Markdown
Member

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().

Refs: #28686

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

@nodejs-github-botnodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 18, 2019
@ronag
ronagforce-pushed the http-client-destroy-stream branch 2 times, most recently from 595b846 to 4a82370CompareAugust 18, 2019 13:03
@ronag

ronag commented Aug 18, 2019

Copy link
Copy Markdown
MemberAuthor

@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?

@ronag
ronagforce-pushed the http-client-destroy-stream branch 21 times, most recently from 9223798 to f904ea8CompareAugust 18, 2019 14:14
Comment threadlib/_http_client.js Outdated
@ronag
ronagforce-pushed the http-client-destroy-stream branch 4 times, most recently from 5f4f9d6 to 19425c2CompareAugust 18, 2019 15:13
@ronag
ronagforce-pushed the http-client-destroy-stream branch 2 times, most recently from 2dd8852 to 8ddc326CompareAugust 20, 2019 10:49
Comment threadlib/_http_client.js Outdated
@ronag

Copy link
Copy Markdown
MemberAuthor

OK, I think I'm very happy with this now.

@vsemozhetbytvsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doc nits.

Comment threaddoc/api/deprecations.md Outdated
Comment threaddoc/api/deprecations.md Outdated
Comment threaddoc/api/deprecations.md Outdated
Comment threaddoc/api/deprecations.md Outdated
Comment threaddoc/api/deprecations.md Outdated
Comment threaddoc/api/deprecations.md Outdated
Comment threaddoc/api/http.md Outdated
Comment threaddoc/api/http.md Outdated
Comment threaddoc/api/http.md Outdated
Comment threaddoc/api/deprecations.md Outdated
Comment threadlib/_http_client.js Outdated
Comment threadlib/_http_client.js Outdated
@ronag

Copy link
Copy Markdown
MemberAuthor

fixed @jasnell comments and a failing test

@ronagronag mentioned this pull request Aug 24, 2019
4 tasks
@mscdex

Copy link
Copy Markdown
Contributor

Bad rebase?

@ronag

Copy link
Copy Markdown
MemberAuthor

fixed bad rebase and new conflicts

Comment threadlib/_http_client.js Outdated
@ronag

ronag commented Aug 24, 2019

Copy link
Copy Markdown
MemberAuthor

this.aborted cannot be strictly an alias to destroyed since it can be set by the user. Fixed.

Comment threadlib/_http_client.js Outdated
@ronag

Copy link
Copy Markdown
MemberAuthor

@mcollina: Any further changes?

@Trott: This needs more reviews?

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment threadlib/_http_client.js Outdated
@mcollina

Copy link
Copy Markdown
Member

cc @nodejs/tsc

@Trott

Copy link
Copy Markdown
Member

(Needs a rebase.)

@ronag

Copy link
Copy Markdown
MemberAuthor

@Trott rebased

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some more tests about this behavior change?

Comment threadtest/parallel/test-http-client-set-timeout.js Outdated
Comment threadtest/parallel/test-http-client-close-event.js Outdated
Comment threadlib/_http_client.js Outdated
@ronag

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadlib/_http_client.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ronagronagSep 24, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronagronagSep 24, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Test added

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ronagronagSep 25, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should strive to not break current users

How does this break current users?

@ronag

ronag commented Sep 24, 2019

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
MemberAuthor

This PR can probably be easier implemented with #29656

@ronag

Copy link
Copy Markdown
MemberAuthor

This is blocked by #29656

@ronag

Copy link
Copy Markdown
MemberAuthor

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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpIssues or PRs related to the http subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@ronag@mscdex@mcollina@Trott@nodejs-github-bot@jasnell@wa-Nadoo@lpinca@vsemozhetbyt