http: emit close on socket re-use #28685

ronag · 2019-07-14T19:46:05Z

When using agents we currently don't emit 'close' on req & res.

I will need a little help here... I don't think I quite got it right.

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

ronag · 2019-07-14T19:52:59Z

@benjamingr I think I'm getting close to spamming issues and PR's now... I'll stop here for now until some of the other's get merged or closed. If there anyway I can help speed up the process feel free to let me know. Matteo has my mail if you want to setup any phone chat.

benjamingr · 2019-07-15T07:02:53Z

@benjamingr I think I'm getting close to spamming issues and PR's now... I'll stop here for now until some of the other's get merged or closed.

You are not spamming PRs or issues everything here has substance, it is getting slow uptake because there aren't many people who understand this area in the code not because the changes are bad. Please keep making PRs :]

lib/_http_client.js

jasnell · 2019-07-15T14:48:06Z

Defensively marking as semver-major as this has a non-zero chance of breaking at least someone.

ronag · 2019-07-15T17:07:47Z

tests added and bugs fixed

mcollina

LGTM

nodejs-github-bot · 2019-08-03T10:20:57Z

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

Trott · 2019-08-04T03:48:28Z

Needs a CITGM run but CITGM is kinda-sorta blocked by standard-things/esm#821 right now (as that issue causes a number of failures in CITGM, or at least that's my understanding).

ronag · 2019-09-04T22:06:28Z

@Trott: can we get that CITGM on this one?

nodejs-github-bot · 2019-09-05T03:13:40Z

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

Trott · 2019-09-05T03:14:01Z

CITGM (queued): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1995/

ronag · 2019-09-11T14:43:15Z

@Trott can we land this?

mcollina · 2019-09-11T16:23:39Z

We can't, this is breaking Ember:

 Command failed: yarn install --non-interactive
 warning You are using Node "13.0.0-pre" which is not supported and may encounter bugs or unexpected behavior. Yarn supports the following semver range: "^4.8.0 || ^5.7.0 || ^6.2.2 || >=8.0.0"
 warning package.json: No license field
 warning foo: No license field
 events.js:484
   const copy = new Array(n);
                ^
 RangeError: Maximum call stack size exceeded
     at arrayClone (events.js:484:16)
     at IncomingMessage.emit (events.js:212:23)
     at responseKeepAlive (_http_client.js:613:13)
     at IncomingMessage.responseOnEnd (_http_client.js:645:5)
     at IncomingMessage.emit (events.js:214:15)
     at IncomingMessage.<anonymous> (/home/iojs/build/workspace/citgm-smoker/nodes/fedora-last-latest-x64/smoker/lib/node_modules/citgm/node_modules/yarn/lib/cli.js:141275:23)
     at IncomingMessage.emit (events.js:214:15)
     at responseKeepAlive (_http_client.js:613:13)
     at IncomingMessage.responseOnEnd (_http_client.js:645:5)
     at IncomingMessage.emit (events.js:214:15)
 yarn install v1.17.3
 [1/4] Resolving packages...
 Error: Command failed: yarn install --non-interactive
 warning You are using Node "13.0.0-pre" which is not supported and may encounter bugs or unexpected behavior. Yarn supports the following semver range: "^4.8.0 || ^5.7.0 || ^6.2.2 || >=8.0.0"
 warning package.json: No license field
 warning foo: No license field

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1995/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/ember_cli_v3_12_0/

ronag · 2019-09-11T16:50:49Z

Ember? Interesting. Would you like me to take a look?

mcollina · 2019-09-11T17:12:22Z

yes please! Il giorno mer 11 set 2019 alle ore 18:52 Robert Nagy < notifications@github.com> ha scritto:

…

Ember? Interesting. Would you like me to take a look? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#28685?email_source=notifications&email_token=AAAMXY2TL4DONOEXFWVHPCDQJEO6NA5CNFSM4IDRZ6A2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6PEUHY#issuecomment-530467359>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMXY2VRRQ4IGP5JZ5PMVLQJEO6NANCNFSM4IDRZ6AQ> .

ronag · 2019-09-11T18:37:21Z

It's actually yarn that fails here:

    // Be a good stream and emit end when the response is finished.
    // Hack to emit end on close because of a core bug that never fires end
    response.on('close', function () {
      if (!self._ended) {
        self.response.emit('end')
      }
    })

    response.once('end', function () {
      self._ended = true
    })

Looks like a bug in yarn. It should be checking res.req._ended.

ronag · 2019-09-23T23:07:55Z

This needed some refactoring to make CITGM pass and resolve conflicts. Will request re-reviews once I get an ok from Travis.

ronag · 2019-09-23T23:08:30Z

lib/_http_client.js

@@ -629,7 +630,7 @@ function responseOnEnd() {
        socket.end();
    }
    assert(!socket.writable);
-  } else if (req.finished) {
+  } else if (req.finished && !this.aborted) {


If response is aborted then the socket is closed and there is nothing to keep alive.

ronag · 2019-09-23T23:18:04Z

lib/_http_client.js

+  req.emit('close');
+  if (req.res) {
+    req.res.emit('close');
+  }


Had to move the close emitters here to ensure they are emitted after all userland 'end' handlers have completed.

nodejs-github-bot · 2019-10-05T18:36:34Z

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

ronag · 2019-10-13T22:50:05Z

@Trott ready to land?

nodejs-github-bot · 2019-10-13T22:54:39Z

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

Trott · 2019-10-13T22:55:26Z

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

Trott · 2019-10-13T22:57:00Z

@mcollina @jasnell A quick re-review might not be a bad idea, given that some stuff has moved/ changed.

nodejs-github-bot · 2019-10-14T00:01:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26003/ ✔️

Trott · 2019-10-14T21:27:03Z

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

CITGM results look good.

Trott · 2019-10-14T21:28:39Z

@nodejs/http @nodejs/collaborators Some final reviews on this one?

PR-URL: #28685 Refs: #28684 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2019-10-15T23:53:24Z

Landed in d247a8e

nodejs/node#28685 yarnpkg/yarn#7542

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 14, 2019

ronag force-pushed the fix-req-res-agent-close branch from 05c108f to 5c11992 Compare July 14, 2019 19:48

benjamingr reviewed Jul 15, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 15, 2019

ronag force-pushed the fix-req-res-agent-close branch from 5c11992 to de246fc Compare July 15, 2019 17:07

ronag force-pushed the fix-req-res-agent-close branch 4 times, most recently from 286fc50 to 15f46b5 Compare July 16, 2019 07:48

mcollina approved these changes Jul 16, 2019

View reviewed changes

benjamingr approved these changes Jul 17, 2019

View reviewed changes

jasnell approved these changes Jul 17, 2019

View reviewed changes

trivikr approved these changes Jul 17, 2019

View reviewed changes

Trott approved these changes Aug 4, 2019

View reviewed changes

ronag mentioned this pull request Aug 6, 2019

http: ClientRequest streamlike #29002

Closed

9 tasks

ronag force-pushed the fix-req-res-agent-close branch from 15f46b5 to ff6911b Compare August 9, 2019 16:08

ronag force-pushed the fix-req-res-agent-close branch from 41e33d0 to e224aba Compare September 23, 2019 23:06

ronag commented Sep 23, 2019

View reviewed changes

ronag force-pushed the fix-req-res-agent-close branch 4 times, most recently from 41e6890 to 3c7d3f9 Compare September 23, 2019 23:13

fixup: pass test

55d1d3d

ronag force-pushed the fix-req-res-agent-close branch from 3c7d3f9 to 55d1d3d Compare September 23, 2019 23:14

ronag commented Sep 23, 2019

View reviewed changes

ronag requested review from Trott, benjamingr and trivikr October 4, 2019 06:30

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2019

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 14, 2019

Trott added this to the 13.0.0 milestone Oct 14, 2019

trivikr approved these changes Oct 15, 2019

View reviewed changes

Trott closed this Oct 15, 2019

BethGriggs mentioned this pull request Mar 10, 2020

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

Merged

vladat added a commit to usecanoe/request that referenced this pull request Jul 2, 2020

Fix 'Call Stack Size Exceeded' in request.js

636bcdf

nodejs/node#28685 yarnpkg/yarn#7542

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: emit close on socket re-use #28685

http: emit close on socket re-use #28685

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

benjamingr commented Jul 15, 2019

jasnell commented Jul 15, 2019

ronag commented Jul 15, 2019

mcollina left a comment

nodejs-github-bot commented Aug 3, 2019

Trott commented Aug 4, 2019

ronag commented Sep 4, 2019

nodejs-github-bot commented Sep 5, 2019

Trott commented Sep 5, 2019

ronag commented Sep 11, 2019

mcollina commented Sep 11, 2019

ronag commented Sep 11, 2019

mcollina commented Sep 11, 2019 via email

ronag commented Sep 11, 2019 •

edited

Loading

ronag commented Sep 23, 2019

ronag Sep 23, 2019

ronag Sep 23, 2019

nodejs-github-bot commented Oct 5, 2019

ronag commented Oct 13, 2019

nodejs-github-bot commented Oct 13, 2019

Trott commented Oct 13, 2019

Trott commented Oct 13, 2019

nodejs-github-bot commented Oct 14, 2019 •

edited by Trott

Loading

Trott commented Oct 14, 2019

Trott commented Oct 14, 2019

Trott commented Oct 15, 2019

http: emit close on socket re-use #28685

http: emit close on socket re-use #28685

Conversation

ronag commented Jul 14, 2019 • edited Loading

Checklist

ronag commented Jul 14, 2019 • edited Loading

benjamingr commented Jul 15, 2019

jasnell commented Jul 15, 2019

ronag commented Jul 15, 2019

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 3, 2019

Trott commented Aug 4, 2019

ronag commented Sep 4, 2019

nodejs-github-bot commented Sep 5, 2019

Trott commented Sep 5, 2019

ronag commented Sep 11, 2019

mcollina commented Sep 11, 2019

ronag commented Sep 11, 2019

mcollina commented Sep 11, 2019 via email

ronag commented Sep 11, 2019 • edited Loading

ronag commented Sep 23, 2019

ronag Sep 23, 2019

Choose a reason for hiding this comment

ronag Sep 23, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 5, 2019

ronag commented Oct 13, 2019

nodejs-github-bot commented Oct 13, 2019

Trott commented Oct 13, 2019

Trott commented Oct 13, 2019

nodejs-github-bot commented Oct 14, 2019 • edited by Trott Loading

Trott commented Oct 14, 2019

Trott commented Oct 14, 2019

Trott commented Oct 15, 2019

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Sep 11, 2019 •

edited

Loading

nodejs-github-bot commented Oct 14, 2019 •

edited by Trott

Loading