http: align OutgoingMessage and ClientRequest destroy #32148

ronag · 2020-03-08T11:42:57Z

Added .destroyed property to OutgoingMessage and ClientRequest
to align with streams.

Fixed ClientRequest.destroy to dump res and re-use socket in agent
pool aligning it with abort.

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

Added .destroyed property to OutgoingMessage and ClientRequest to align with streams. Fixed ClientRequest.destroy to dump res and re-use socket in agent pool aligning it with abort.

ronag · 2020-03-08T11:44:08Z

lib/_http_client.js

+      }
+
+      if (!this.aborted && !err) {
+        err = connResetException('socket hang up');


Note, destroy() will emit ECONNRESET while abort won't.

nodejs-github-bot · 2020-03-08T11:44:20Z

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

ronag · 2020-03-08T11:54:29Z

After this we might want to consider deprecating abort.

szmarczak · 2020-03-08T13:32:11Z

There are some changes needed to be made, but I'm not at home atm, will comment in an hour.

lib/internal/streams/destroy.js

lib/_http_client.js

ronag · 2020-03-08T14:01:02Z

Removed the risky change with destroyer.

ronag · 2020-03-08T14:02:14Z

@nodejs/web-server-frameworks

lib/_http_outgoing.js

lib/_http_client.js

This reverts commit 6f11b24.

nodejs-github-bot · 2020-03-08T22:13:41Z

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

szmarczak

Looks good! Awesome :D

szmarczak · 2020-03-08T22:38:20Z

I think it's a good idea to replace

node/lib/_http_incoming.js

Lines 118 to 124 in 7bb4f95

    
           // It's possible that the socket will be destroyed, and removed from 
        
           // any messages, before ever calling this.  In that case, just skip 
        
           // it, since something else is destroying this connection anyway. 
        
           IncomingMessage.prototype.destroy = function destroy(error) { 
        
             if (this.socket) 
        
               this.socket.destroy(error); 
        
           };

with

IncomingMessage.prototype.destroy = function destroy(error) {
  if (this.req)
    this.req.destroy(error);
};

ronag · 2020-03-08T22:41:36Z

@szmarczak See #32153 for that.

codecov-io · 2020-03-09T03:15:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4d93e10). Click here to learn what that means.
The diff coverage is 94.11%.

@@            Coverage Diff            @@
##             master   #32148   +/-   ##
=========================================
  Coverage          ?   97.04%           
=========================================
  Files             ?      197           
  Lines             ?    65027           
  Branches          ?        0           
=========================================
  Hits              ?    63108           
  Misses            ?     1919           
  Partials          ?        0

Impacted Files	Coverage Δ
lib/wasi.js	`98.49% <94.11%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d93e10...f24b4a7. Read the comment docs.

ronag · 2020-03-09T12:49:43Z

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

ronag · 2020-03-10T08:19:37Z

@nodejs/http

ronag · 2020-03-10T11:57:29Z

@nodejs/tsc

lib/internal/streams/destroy.js

mcollina

lgtm

Added .destroyed property to OutgoingMessage and ClientRequest to align with streams. Fixed ClientRequest.destroy to dump res and re-use socket in agent pool aligning it with abort. PR-URL: #32148 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ronag · 2020-03-10T17:16:16Z

Landed in 173d044

szmarczak · 2020-03-10T19:28:30Z

Will this be backported to v13.x?

ronag · 2020-03-10T19:30:04Z

@szmarczak Not as it is right now, it's been labeled semver-major.

http: align OutgoingMessage and ClientRequest destroy

1b275e0

Added .destroyed property to OutgoingMessage and ClientRequest to align with streams. Fixed ClientRequest.destroy to dump res and re-use socket in agent pool aligning it with abort.

ronag added 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. labels Mar 8, 2020

ronag requested review from mcollina and jasnell March 8, 2020 11:42

ronag commented Mar 8, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

ronag force-pushed the http-client-destroy4 branch 2 times, most recently from b795096 to 36a003a Compare March 8, 2020 12:08

fixup: use destroy when available

54de489

ronag force-pushed the http-client-destroy4 branch from 36a003a to 54de489 Compare March 8, 2020 12:08

ronag commented Mar 8, 2020

View reviewed changes

lib/internal/streams/destroy.js Outdated Show resolved Hide resolved

ronag commented Mar 8, 2020

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

fixup: reduce changes

88ce624

ronag force-pushed the http-client-destroy4 branch from dae0286 to 88ce624 Compare March 8, 2020 14:01

fixup

2746f70

ronag force-pushed the http-client-destroy4 branch 2 times, most recently from 844adca to 97bfd14 Compare March 8, 2020 14:21

fixup: make more similar to original

0998e9b

ronag force-pushed the http-client-destroy4 branch from 97bfd14 to 0998e9b Compare March 8, 2020 14:21

szmarczak reviewed Mar 8, 2020

View reviewed changes

lib/_http_outgoing.js Show resolved Hide resolved

lib/_http_client.js Outdated Show resolved Hide resolved

lib/_http_client.js Outdated Show resolved Hide resolved

fixup: refactor for TODO

0245e4e

ronag force-pushed the http-client-destroy4 branch from 92207e2 to 0245e4e Compare March 8, 2020 15:56

fixup: reuse if not headersSent

6f11b24

ronag force-pushed the http-client-destroy4 branch from a5199da to 6f11b24 Compare March 8, 2020 16:59

ronag commented Mar 8, 2020

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ronag added 2 commits March 8, 2020 19:00

Revert "fixup: reuse if not headersSent"

2750006

This reverts commit 6f11b24.

fixup: test

f24b4a7

szmarczak approved these changes Mar 8, 2020

View reviewed changes

addaleax approved these changes Mar 9, 2020

View reviewed changes

szmarczak mentioned this pull request Mar 10, 2020

Broken with Node.js ≥ 13.10.0 sindresorhus/got#1107

Closed

2 tasks

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2020

mcollina reviewed Mar 10, 2020

View reviewed changes

lib/internal/streams/destroy.js Show resolved Hide resolved

mcollina approved these changes Mar 10, 2020

View reviewed changes

ronag closed this Mar 10, 2020

BethGriggs mentioned this pull request Mar 17, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

ronag mentioned this pull request Apr 16, 2020

http: doc deprecate abort and improve docs #32807

Closed

4 tasks

mastermatt mentioned this pull request May 8, 2020

fix(ClientRequest): Use native abort and destroy nock/nock#2000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: align OutgoingMessage and ClientRequest destroy #32148

http: align OutgoingMessage and ClientRequest destroy #32148

ronag commented Mar 8, 2020 •

edited

Loading

ronag Mar 8, 2020

nodejs-github-bot commented Mar 8, 2020

This comment has been minimized.

ronag commented Mar 8, 2020

szmarczak commented Mar 8, 2020

ronag commented Mar 8, 2020

ronag commented Mar 8, 2020

This comment has been minimized.

nodejs-github-bot commented Mar 8, 2020

szmarczak left a comment

szmarczak commented Mar 8, 2020 •

edited

Loading

ronag commented Mar 8, 2020

codecov-io commented Mar 9, 2020

ronag commented Mar 9, 2020 •

edited

Loading

ronag commented Mar 10, 2020

ronag commented Mar 10, 2020

mcollina left a comment

ronag commented Mar 10, 2020

szmarczak commented Mar 10, 2020

ronag commented Mar 10, 2020

http: align OutgoingMessage and ClientRequest destroy #32148

http: align OutgoingMessage and ClientRequest destroy #32148

Conversation

ronag commented Mar 8, 2020 • edited Loading

Checklist

ronag Mar 8, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 8, 2020

This comment has been minimized.

ronag commented Mar 8, 2020

szmarczak commented Mar 8, 2020

ronag commented Mar 8, 2020

ronag commented Mar 8, 2020

This comment has been minimized.

nodejs-github-bot commented Mar 8, 2020

szmarczak left a comment

Choose a reason for hiding this comment

szmarczak commented Mar 8, 2020 • edited Loading

ronag commented Mar 8, 2020

codecov-io commented Mar 9, 2020

Codecov Report

ronag commented Mar 9, 2020 • edited Loading

ronag commented Mar 10, 2020

ronag commented Mar 10, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Mar 10, 2020

szmarczak commented Mar 10, 2020

ronag commented Mar 10, 2020

ronag commented Mar 8, 2020 •

edited

Loading

szmarczak commented Mar 8, 2020 •

edited

Loading

ronag commented Mar 9, 2020 •

edited

Loading