streams: support unlimited synchronous cork/uncork cycles #6164

mcollina · 2016-04-12T10:37:50Z

Checklist

tests and code linting passes
a test and/or benchmark is included

Affected core subsystem(s)

streams, writable

Description of change

net streams can request multiple chunks to be written in a synchronous
fashion. If this is combined with cork/uncork, en error is currently
thrown because of a regression introduced in:
89aeab9
(#4354).

I am not creating a full linked list anyway, because that is not needed in any "normal" operation of the stream, and I think multiple cork/uncork synchronously it is a very edge case: we can possibly waste some objects and callbacks.
Just to clarify, we are supporting a "feature" that can easily lead to event loop starvation and memory leaks, because for each synchronous cork/uncork cycle, we are adding a task in the nextTick queue:

node/lib/_stream_writable.js

Line 344 in 819b2d3

process.nextTick(afterWrite, stream, state, finished, cb);

.

Fixes: #6154.

mcollina · 2016-04-12T10:38:02Z

@jfhbrook are you able to test this?

mcollina · 2016-04-12T10:38:54Z

cc @nodejs/streams please review asap.

@thealphanerd you might want to include this one for #6149.

calvinmetcalf · 2016-04-12T11:33:09Z

lib/_stream_writable.js

@@ -387,12 +387,17 @@ function clearBuffer(stream, state) {

    doWrite(stream, state, true, state.length, buffer, '', holder.finish);

-    // doWrite is always async, defer these to save a bit of time
+    // doWrite is almost always async, defer these to save a bit of time
    // as the hot path ends with doWrite
    state.pendingcb++;
    state.lastBufferedRequest = null;
    state.corkedRequestsFree = holder.next;


probably want to remove this line as well

mafintosh · 2016-04-12T12:28:05Z

Other than @calvinmetcalf's comment this is LGTM from em

net streams can request multiple chunks to be written in a synchronous fashion. If this is combined with cork/uncork, en error is currently thrown because of a regression introduced in: nodejs@89aeab9 (nodejs#4354). Fixes: nodejs#6154.

mcollina · 2016-04-12T12:41:03Z

I have updated it, mainly cleaning things up. Let me know what you think cc @mafintosh @calvinmetcalf.

mcollina · 2016-04-12T13:08:37Z

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

mcollina · 2016-04-12T13:08:49Z

(how do I launch CITGM?)

mscdex · 2016-04-12T13:28:21Z

@mcollina The thealphanerd-smoker task. Started one here: https://ci.nodejs.org/job/thealphanerd-smoker/195/

mcollina · 2016-04-12T13:28:31Z

Another CI: https://ci.nodejs.org/job/node-test-pull-request/2247 (previous one missing the "safe" flag).

mcollina · 2016-04-12T14:14:33Z

One failure (seems spurious) on ARM https://ci.nodejs.org/job/node-test-binary-arm/1691/RUN_SUBSET=6,nodes=pi2-raspbian-wheezy/tapTestReport/

jfhbrook · 2016-04-12T14:28:36Z

@jfhbrook are you able to test this?

A little hard because we're only seeing this in production, but I'll see what we can do.

mcollina · 2016-04-12T14:55:02Z

CIGTM passes.

MylesBorins · 2016-04-12T15:00:59Z

How long will it take for this to land in v5?

mcollina · 2016-04-12T15:03:10Z

Done for me, awaiting LGTMs. CI and CIGTM passes. Today or tomorrow I presume.

benjamingr · 2016-04-12T15:19:15Z

test/parallel/test-net-sync-cork.js

+      res = conn.write(buf);
+      conn.uncork();
+    }
+    assert.equal(i, N);


maybe also assert res is still true, since it could have been false in the last iteration.

You are right, it's not really an assert on that. I was also afraid of putting the assert in i, as it could be spurious (it depends on OS buffers and the like). I would avoid to stretching luck, I picked 100 as a random number that was working on my box.

100 seems reasonable, a comment there would be nice but I guess it's pretty straightforward as is.

benjamingr · 2016-04-12T15:20:19Z

LGTM, nice fix.

jfhbrook · 2016-04-12T16:09:01Z

How long will it take for this to land in v5?

More importantly, v4?

benjamingr · 2016-04-12T17:40:33Z

@jfhbrook typically pull requests wait for 2 days before being landed so all collaborators have a chance to look at them. After that the road to v4 and v5 is usually very fast. The fix is good and it has already been LGTMd by 2 collaborators so it shouldn't be more than 2 days unless new information is discovered. Of course - in cases of urgency pull requests might be merged sooner, and also - you can always take the changes here - apply them to the v4 branch and build it if you need this fix faster than that.

mcollina · 2016-04-13T07:01:50Z

If there aren't any other concern, I would merge it on Thursday morning (CEST).

mafintosh · 2016-04-13T10:07:50Z

@mcollina LGTM

mafintosh · 2016-04-13T10:10:10Z

@mcollina also great job on the fast turn around!

jasnell · 2016-04-13T14:59:46Z

LGTM

jfhbrook · 2016-04-13T19:06:09Z

@mcollina just a heads up, we are in fact floating this patch in prod right now, so far no breakage \o/

This is expected to land in 4.4.4 right? Just trying to track the work around not having to float patches anymore :)

benjamingr · 2016-04-13T19:23:59Z

@jfhbrook if you're eager and want to help you're welcome to open a pull request backporting this patch against the v4-staging branch. Contributions are always welcome.

jfhbrook · 2016-04-13T19:25:16Z

Sure. Would that be helpful? Like in our case I just did a cherry-pick

net streams can request multiple chunks to be written in a synchronous fashion. If this is combined with cork/uncork, en error is currently thrown because of a regression introduced in: 89aeab9 (#4354). Fixes: #6154 PR-URL: #6164 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behavior was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) nodejs#5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) nodejs#6279 * update ESLint to 2.7.0 (silverwind) nodejs#6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) nodejs#6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) nodejs#6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) nodejs#6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) nodejs#6090 src: * add SIGINFO to supported signals (James Reggio) nodejs#6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) nodejs#6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) nodejs#6069 PR-URL: nodejs#6322

net streams can request multiple chunks to be written in a synchronous fashion. If this is combined with cork/uncork, en error is currently thrown because of a regression introduced in: 89aeab9 (#4354). Fixes: #6154 PR-URL: #6164 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

net streams can request multiple chunks to be written in a synchronous fashion. If this is combined with cork/uncork, en error is currently thrown because of a regression introduced in: 89aeab9 (#4354). Fixes: #6154 PR-URL: #6164 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mcollina added stream Issues and PRs related to the stream subsystem. lts-watch-v4.x labels Apr 12, 2016

calvinmetcalf reviewed Apr 12, 2016
View reviewed changes

mcollina force-pushed the unlimited-corked-requests branch from f91d9b5 to ce2a040 Compare April 12, 2016 12:39

mcollina mentioned this pull request Apr 12, 2016

V4.4.3 proposal #6149

Merged

benjamingr reviewed Apr 12, 2016
View reviewed changes

MylesBorins removed the dont-land-on-v4.x label Apr 14, 2016

jfhbrook mentioned this pull request Apr 15, 2016

Nightly and/or RC builds for LTS patch releases nodejs/Release#93

Closed

MylesBorins mentioned this pull request Apr 20, 2016

V5.10.2 proposal #6311

Closed

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Apr 30, 2016

MylesBorins mentioned this pull request May 4, 2016

v4.4.4 proposal #6583

Merged

mcollina deleted the unlimited-corked-requests branch May 18, 2016 14:47

mcollina mentioned this pull request Jan 17, 2018

stream: delete redundant code #18145

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streams: support unlimited synchronous cork/uncork cycles #6164

streams: support unlimited synchronous cork/uncork cycles #6164

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

calvinmetcalf Apr 12, 2016

mafintosh commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

mscdex commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

jfhbrook commented Apr 12, 2016

mcollina commented Apr 12, 2016

MylesBorins commented Apr 12, 2016

mcollina commented Apr 12, 2016

benjamingr Apr 12, 2016

mcollina Apr 12, 2016

benjamingr Apr 13, 2016

benjamingr commented Apr 12, 2016

jfhbrook commented Apr 12, 2016

benjamingr commented Apr 12, 2016

mcollina commented Apr 13, 2016

mafintosh commented Apr 13, 2016

mafintosh commented Apr 13, 2016

jasnell commented Apr 13, 2016

jfhbrook commented Apr 13, 2016

benjamingr commented Apr 13, 2016

jfhbrook commented Apr 13, 2016

streams: support unlimited synchronous cork/uncork cycles #6164

streams: support unlimited synchronous cork/uncork cycles #6164

Conversation

mcollina commented Apr 12, 2016

Checklist

Affected core subsystem(s)

Description of change

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

calvinmetcalf Apr 12, 2016

Choose a reason for hiding this comment

mafintosh commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

mscdex commented Apr 12, 2016

mcollina commented Apr 12, 2016

mcollina commented Apr 12, 2016

jfhbrook commented Apr 12, 2016

mcollina commented Apr 12, 2016

MylesBorins commented Apr 12, 2016

mcollina commented Apr 12, 2016

benjamingr Apr 12, 2016

Choose a reason for hiding this comment

mcollina Apr 12, 2016

Choose a reason for hiding this comment

benjamingr Apr 13, 2016

Choose a reason for hiding this comment

benjamingr commented Apr 12, 2016

jfhbrook commented Apr 12, 2016

benjamingr commented Apr 12, 2016

mcollina commented Apr 13, 2016

mafintosh commented Apr 13, 2016

mafintosh commented Apr 13, 2016

jasnell commented Apr 13, 2016

jfhbrook commented Apr 13, 2016

benjamingr commented Apr 13, 2016

jfhbrook commented Apr 13, 2016