Skip to content

Conversation

@ronag
Copy link
Member

@ronagronag commented Jul 14, 2019

ClientRequest.destroy() should be the same as abort(). Make ClientRequest more streamlike by deprecating abort().

If request has completed it cannot be aborted.

This also allows us to replace a lot of edge case code (e.g. isRequest) that has to call abort for ClientRequest while everything else is just destroy.

Calling destroy previously instead of abort might have some weird behaviour since abort seems to take a lot more stuff into account e.g. req.agent.

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

NOTE TO SELF: look into the callback

@nodejs-github-botnodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 14, 2019
@ronagronagforce-pushed the http-client-destroy2 branch 4 times, most recently from 7962df1 to cdd194eCompareJuly 14, 2019 19:27
@addaleaxaddaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 14, 2019
@ronagronagforce-pushed the http-client-destroy2 branch 3 times, most recently from 673aa19 to 6bd3be5CompareJuly 14, 2019 20:05
@ronagronagforce-pushed the http-client-destroy2 branch 2 times, most recently from a450164 to 54c460aCompareJuly 14, 2019 20:24
@ronagronagforce-pushed the http-client-destroy2 branch 2 times, most recently from 9f97795 to d0101f8CompareJuly 15, 2019 14:14
@ronag
Copy link
MemberAuthor

There is more work to be done (e.g. the TODO in this PR) in terms of making ClientRequest and OutgoingMessage more stream like. But I suggest that be done in future PR's.

@ronagronagforce-pushed the http-client-destroy2 branch from d0101f8 to ab00509CompareJuly 15, 2019 18:00
@ronag
Copy link
MemberAuthor

Fixed failing test

@ronag
Copy link
MemberAuthor

ping @benjamingr

@ronagronagforce-pushed the http-client-destroy2 branch 6 times, most recently from 9f8fb09 to 3bbfbdeCompareJuly 15, 2019 20:22
@TrottTrott mentioned this pull request Jul 17, 2019
2 tasks
@ronagronagforce-pushed the http-client-destroy2 branch from c9baec5 to 3211fe8CompareAugust 2, 2019 09:55
@trivikrtrivikr requested a review from lpincaAugust 2, 2019 17:50
@ronagronag mentioned this pull request Aug 6, 2019
9 tasks
@ronagronagforce-pushed the http-client-destroy2 branch from 3211fe8 to 6f95d70CompareAugust 9, 2019 16:08
@ronag
Copy link
MemberAuthor

ronag commented Aug 9, 2019

@Trott: This is no longer blocked

@TrottTrott removed the blocked PRs that are blocked by other issues or PRs. label Aug 9, 2019
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
MemberAuthor

@Trott: this looks ready

@ronag
Copy link
MemberAuthor

@lpinca you good with this?

Copy link
Member

@lpincalpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

@lpinca
Copy link
Member

I think this requires a CITGM run. The breaking changes are big enough.

@ronag
Copy link
MemberAuthor

RSLGTM

lol, what? :D

@lpinca
Copy link
Member

Rubber Stamp LGTM :)

@ronag
Copy link
MemberAuthor

@Trott CITGM please

@lpinca
Copy link
Member

Type: Documentation-only
[`ClientRequest.destroy()`][] should be the same as
[`ClientRequest.abort()`][]. Make ClientRequest more streamlike by deprecating
Copy link
Member

Choose a reason for hiding this comment

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

Optional suggestion:

Suggested change
[`ClientRequest.abort()`][]. Make ClientRequest more streamlike by deprecating
[`ClientRequest.abort()`][]. Make ClientRequest more stream-like by deprecating

@Trott
Copy link
Member

CITGM looks good but this does need a rebase.

@ronag
Copy link
MemberAuthor

@Trott: this is missing deprecation idx (i.e. DEP0XXX) which makes merging harder... 0daec61... is that a mistake?

@ronagronag mentioned this pull request Aug 18, 2019
4 tasks
@ronag
Copy link
MemberAuthor

I think #29192 is a much more elegant solution, although a bit more risky...

@ronagronag mentioned this pull request Aug 18, 2019
4 tasks
@ronag
Copy link
MemberAuthor

ronag commented Aug 20, 2019

@mcollina, @lpinca (someone else?): unless you have an objection I would prefer to close this PR in favor of #29192.

@Trott: Otherwise, I would like to remove the change where we swallow any error after destroy()/abort() until the conversation in #29197 is resolved.

@mcollinamcollina dismissed their stale reviewAugust 20, 2019 08:24

This PR needs mode thoughts.

@ronag
Copy link
MemberAuthor

Closing in favor of #29192 which should not be as controversial. Will open a new PR if required once there is consensus.

@ronagronag closed this Aug 20, 2019
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.

7 participants

@ronag@mcollina@nodejs-github-bot@lpinca@Trott@addaleax@vsemozhetbyt