Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: ClientRequest.abort is destroy #28683

Closed
wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag 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-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
@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
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the http-client-destroy2 branch 2 times, most recently from a450164 to 54c460a Compare July 14, 2019 20:24
doc/api/http.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the http-client-destroy2 branch 2 times, most recently from 9f97795 to d0101f8 Compare July 15, 2019 14:14
@ronag
Copy link
Member Author

ronag commented Jul 15, 2019

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
Copy link
Member Author

ronag commented Jul 15, 2019

Fixed failing test

@ronag
Copy link
Member Author

ronag commented Jul 15, 2019

ping @benjamingr

lib/_http_client.js Show resolved Hide resolved
lib/_http_client.js Show resolved Hide resolved
@ronag ronag force-pushed the http-client-destroy2 branch 6 times, most recently from 9f8fb09 to 3bbfbde Compare July 15, 2019 20:22
@Trott Trott mentioned this pull request Jul 17, 2019
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
9 tasks
@ronag ronag force-pushed the http-client-destroy2 branch from 3211fe8 to 6f95d70 Compare August 9, 2019 16:08
@ronag
Copy link
Member Author

ronag commented Aug 9, 2019

@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
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott: this looks ready

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@lpinca you good with this?

Copy link
Member

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

lpinca commented Aug 17, 2019

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

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

RSLGTM

lol, what? :D

@lpinca
Copy link
Member

lpinca commented Aug 17, 2019

Rubber Stamp LGTM :)

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott CITGM please

@lpinca
Copy link
Member

lpinca commented Aug 17, 2019

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

Trott commented Aug 17, 2019

CITGM looks good but this does need a rebase.

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@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
4 tasks
@ronag
Copy link
Member Author

ronag commented Aug 18, 2019

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

@ronag ronag mentioned this pull request Aug 18, 2019
4 tasks
@ronag
Copy link
Member Author

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.

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

This PR needs mode thoughts.

@ronag
Copy link
Member Author

ronag commented Aug 20, 2019

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
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
Development

Successfully merging this pull request may close these issues.

7 participants