Skip to content

http: ClientRequest.abort is destroy#28683

Closed
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:http-client-destroy2
Closed

http: ClientRequest.abort is destroy#28683
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:http-client-destroy2

Conversation

@ronag

@ronag ronag commented Jul 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 14, 2019
@ronag ronag force-pushed the http-client-destroy2 branch 4 times, most recently from 7962df1 to cdd194e Compare July 14, 2019 19:27
vsemozhetbyt
vsemozhetbyt reviewed Jul 14, 2019
View reviewed changes
Comment thread doc/api/http.md Outdated
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 14, 2019
@ronag ronag force-pushed the http-client-destroy2 branch 3 times, most recently from 673aa19 to 6bd3be5 Compare July 14, 2019 20:05
addaleax
addaleax approved these changes Jul 14, 2019
View reviewed changes
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/deprecations.md Outdated
@ronag ronag force-pushed the http-client-destroy2 branch 2 times, most recently from a450164 to 54c460a Compare July 14, 2019 20:24
vsemozhetbyt
vsemozhetbyt reviewed Jul 15, 2019
View reviewed changes
Comment thread doc/api/deprecations.md Outdated
vsemozhetbyt
vsemozhetbyt reviewed Jul 15, 2019
View reviewed changes
Comment thread doc/api/deprecations.md Outdated
vsemozhetbyt
vsemozhetbyt reviewed Jul 15, 2019
View reviewed changes
Comment thread doc/api/http.md Outdated
@ronag ronag force-pushed the http-client-destroy2 branch 2 times, most recently from 9f97795 to d0101f8 Compare July 15, 2019 14:14
@ronag

ronag commented Jul 15, 2019

Copy link
Copy Markdown
Member Author

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.

@ronag ronag force-pushed the http-client-destroy2 branch from d0101f8 to ab00509 Compare July 15, 2019 18:00
@ronag

ronag commented Jul 15, 2019

Copy link
Copy Markdown
Member Author

Fixed failing test

@ronag

ronag commented Jul 15, 2019

Copy link
Copy Markdown
Member Author

ping @benjamingr

lpinca
lpinca reviewed Jul 15, 2019
View reviewed changes
Comment thread lib/_http_client.js Outdated
Comment thread lib/_http_client.js Outdated
@ronag ronag force-pushed the http-client-destroy2 branch 6 times, most recently from 9f8fb09 to 3bbfbde Compare July 15, 2019 20:22
lpinca
lpinca reviewed Jul 16, 2019
View reviewed changes
Comment thread lib/_http_client.js Outdated
@ronag ronag mentioned this pull request Jul 16, 2019
Closed
4 tasks
@Trott Trott mentioned this pull request Jul 17, 2019
Closed
2 tasks
@ronag ronag force-pushed the http-client-destroy2 branch from c9baec5 to 3211fe8 Compare August 2, 2019 09:55
@trivikr trivikr requested a review from lpinca August 2, 2019 17:50
@ronag ronag mentioned this pull request Aug 6, 2019
Closed
9 tasks
@ronag ronag force-pushed the http-client-destroy2 branch from 3211fe8 to 6f95d70 Compare August 9, 2019 16:08
@ronag

ronag commented Aug 9, 2019

Copy link
Copy Markdown
Member Author

@Trott: This is no longer blocked

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

nodejs-github-bot commented Aug 9, 2019

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

@Trott: this looks ready

@ronag

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

@lpinca you good with this?

lpinca
lpinca approved these changes Aug 17, 2019
View reviewed changes

@lpinca lpinca 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.

RSLGTM

@lpinca

lpinca commented Aug 17, 2019

Copy link
Copy Markdown
Member

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

@ronag

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

RSLGTM

lol, what? :D

@lpinca

lpinca commented Aug 17, 2019

Copy link
Copy Markdown
Member

Rubber Stamp LGTM :)

@ronag

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

@Trott CITGM please

@lpinca

lpinca commented Aug 17, 2019

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1953/

Trott
Trott reviewed Aug 17, 2019
View reviewed changes
Comment thread doc/api/deprecations.md
Type: Documentation-only

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

@Trott Trott Aug 17, 2019

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.

Optional suggestion:

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

@Trott

Trott commented Aug 17, 2019

Copy link
Copy Markdown
Member

CITGM looks good but this does need a rebase.

@ronag

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

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

@ronag ronag mentioned this pull request Aug 18, 2019
Closed
4 tasks
@ronag

ronag commented Aug 18, 2019

Copy link
Copy Markdown
Member Author

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

@ronag ronag mentioned this pull request Aug 18, 2019
Closed
4 tasks
@ronag

ronag commented Aug 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@mcollina mcollina dismissed their stale review August 20, 2019 08:24

This PR needs mode thoughts.

@ronag

ronag commented Aug 20, 2019

Copy link
Copy Markdown
Member Author

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

@ronag ronag closed this Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina left review comments

+1 more reviewer

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

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