src: refactor HasWriteQueue() away #18019

addaleax · 2018-01-06T21:44:35Z

process: use more direct sync I/O for stdio

This avoids routing writes through the full LibuvStreamWrap
write machinery. In particular, it enables the next commit,
because otherwise the callback passed to _write()
would not be called synchronously for pipes on Windows
(because the latter does not support uv_try_write(),
even for blocking I/O).
src: remove HasWriteQueue()

Tests are passing without it now, and this otherwise makes the
code harder to reason about because the async flag on the
write request object would not be set even though the callback
would still be pending.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src/stream_base

addaleax · 2018-01-06T21:46:43Z

CI: https://ci.nodejs.org/job/node-test-commit/15267/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1194/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/89/ ~~(88 is running HTTP benchmarks which takes forever)~~
Rerun with more runs because the results were somewhat mixed: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/90/

apapirovski

SGTM if the CI is green. When I worked on this last, there were some cross-system issues around having net writes be async with no actual write queue (which is why we used to have the check for writeQueueSize right in _writeGeneric).

addaleax · 2018-01-06T21:53:02Z

@apapirovski I’m not sure if that’s what you’re talking about, but the first commit here works around the fact that, while pipes on Windows can be set to blocking, they always need DoWrite() rather than DoTryWrite(); and the second thing was our main REPL test, where some over-specific reliance on chunk boundaries was refactored away in #17926

apapirovski · 2018-01-06T21:55:17Z

@addaleax It's been too long to remember. The bit re: Windows pipes makes sense and the code looks good to me. I feel like there might've been a different issue on linux, although it might've just been the repl test, as you mentioned. Either way, the code looks good to me. 👍

joyeecheung · 2018-01-09T17:00:19Z

lib/net.js

        (this._handle instanceof Pipe) &&
        process.platform === 'win32') {
      // Make stdout and stderr blocking on Windows
      var err = this._handle.setBlocking(true);
      if (err)
        throw errnoException(err, 'setBlocking');
+
+      this._writev = null;
+      this._write = makeSyncWrite(fd);


Can we assign fd to a symbol property here and do the if logic in Socket.prototype._write to avoid the additional closure?

@joyeecheung I have thought about making fd a property for all net.Sockets, but I’m not sure whether that’s a good idea or not.

We could add a symbol, but for now I’d probably leave the code as it is, especially given that the worst case is that this might create 2 closures for the entire process…

This avoids routing writes through the full LibuvStreamWrap write machinery. In particular, it enables the next commit, because otherwise the callback passed to `_write()` would not be called synchronously for pipes on Windows (because the latter does not support `uv_try_write()`, even for blocking I/O).

Tests are passing without it, and this otherwise makes the code harder to reason about because the `async` flag on the write request object would not be set even though the callback would still be pending.

addaleax · 2018-01-10T20:14:34Z

Did some experimenting to keep the benchmarks happy. I ran these on a largely unused machine, since the benchmark CI is currently blocked, for the combination of parameters that seemed most problematic, with 90 runs to be on the safe side:

$ ./node benchmark/compare.js --runs 90 --new ./node --old ./node-master --set type=utf --set len=10240 --filter net-pipe net | Rscript benchmark/compare.R
[00:15:40|% 100| 1/1 files | 180/180 runs | 1/1 configs]: Done
                                             improvement confidence   p.value
 net/net-pipe.js dur=5 type="utf" len=10240      0.07 %            0.8542763

I’m good with landing this in this state, there does not seem to be any performance difference anymore. (This would fit the symptoms – HasWriteQueue() being false after we did try DoTryWrite() seems like a condition that should be very unlikely. That only happened for not-all-buffers calls to writev(), which would be a kind of thing that should happen frequently in that benchmark, and it’s probably a good idea for performance to make these DoTryWrite() calls anyway.)

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

Could somebody review the last commit here?

Edit: Scheduled benchmark CI @ https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/94/

jasnell · 2018-01-10T20:21:31Z

last commit lgtm

addaleax · 2018-01-14T13:47:02Z

Landed in c84582c...02fef8a

This avoids routing writes through the full LibuvStreamWrap write machinery. In particular, it enables the next commit, because otherwise the callback passed to `_write()` would not be called synchronously for pipes on Windows (because the latter does not support `uv_try_write()`, even for blocking I/O). PR-URL: #18019 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Tests are passing without it, and this otherwise makes the code harder to reason about because the `async` flag on the write request object would not be set even though the callback would still be pending. PR-URL: #18019 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #18019 Reviewed-By: James M Snell <[email protected]>

This broke writing non-ASCII data to the console on Windows because the result would be codepage-dependent. This partially reverts 8b751f7. Fixes: #18189 Refs: #18019 PR-URL: #18214 Fixes: #18189 Refs: #18019 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add two regression tests for stdio over pipes. test-stdio-pipe-access tests if accessing stdio pipe that is being read by another process does not deadlocks Node.js. This was reported in #10836 and was fixed in v8.3.0. The deadlock would happen intermittently, so we run the test 5 times. test-stdio-pipe-redirect tests if redirecting one child process stdin to another process stdout does not crash Node as reported in #17493. It was fixed in #18019. PR-URL: #18614 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This avoids routing writes through the full LibuvStreamWrap write machinery. In particular, it enables the next commit, because otherwise the callback passed to `_write()` would not be called synchronously for pipes on Windows (because the latter does not support `uv_try_write()`, even for blocking I/O). PR-URL: #18019 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Tests are passing without it, and this otherwise makes the code harder to reason about because the `async` flag on the write request object would not be set even though the callback would still be pending. PR-URL: #18019 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #18019 Reviewed-By: James M Snell <[email protected]>

This broke writing non-ASCII data to the console on Windows because the result would be codepage-dependent. This partially reverts 8b751f7. Fixes: #18189 Refs: #18019 PR-URL: #18214 Fixes: #18189 Refs: #18019 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add two regression tests for stdio over pipes. test-stdio-pipe-access tests if accessing stdio pipe that is being read by another process does not deadlocks Node.js. This was reported in #10836 and was fixed in v8.3.0. The deadlock would happen intermittently, so we run the test 5 times. test-stdio-pipe-redirect tests if redirecting one child process stdin to another process stdout does not crash Node as reported in #17493. It was fixed in #18019. PR-URL: #18614 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This broke writing non-ASCII data to the console on Windows because the result would be codepage-dependent. This partially reverts 8b751f7. Fixes: nodejs#18189 Refs: nodejs#18019 PR-URL: nodejs#18214 Fixes: nodejs#18189 Refs: nodejs#18019 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add two regression tests for stdio over pipes. test-stdio-pipe-access tests if accessing stdio pipe that is being read by another process does not deadlocks Node.js. This was reported in nodejs#10836 and was fixed in v8.3.0. The deadlock would happen intermittently, so we run the test 5 times. test-stdio-pipe-redirect tests if redirecting one child process stdin to another process stdout does not crash Node as reported in nodejs#17493. It was fixed in nodejs#18019. PR-URL: nodejs#18614 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

codebytere · 2018-08-02T15:41:01Z

@addaleax should this be backported to v8.x or has it not yet baked sufficiently?

Add two regression tests for stdio over pipes. test-stdio-pipe-access tests if accessing stdio pipe that is being read by another process does not deadlocks Node.js. This was reported in #10836 and was fixed in v8.3.0. The deadlock would happen intermittently, so we run the test 5 times. test-stdio-pipe-redirect tests if redirecting one child process stdin to another process stdout does not crash Node as reported in #17493. It was fixed in #18019. PR-URL: #18614 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2018-08-17T19:00:07Z

I've gone ahead and landed this on v8.x as it fixes a broken test that was already landed

20f6aae was not included as hasWriteQueue did not exist on 8.x

This avoids routing writes through the full LibuvStreamWrap write machinery. In particular, it enables the next commit, because otherwise the callback passed to `_write()` would not be called synchronously for pipes on Windows (because the latter does not support `uv_try_write()`, even for blocking I/O). PR-URL: #18019 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #18019 Reviewed-By: James M Snell <[email protected]>

This broke writing non-ASCII data to the console on Windows because the result would be codepage-dependent. This partially reverts 8b751f7. Fixes: #18189 Refs: #18019 PR-URL: #18214 Fixes: #18189 Refs: #18019 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

addaleax requested a review from apapirovski January 6, 2018 21:44

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 6, 2018

apapirovski approved these changes Jan 6, 2018

View reviewed changes

addaleax added dont-land-on-v4.x labels Jan 6, 2018

bnoordhuis approved these changes Jan 7, 2018

View reviewed changes

jasnell approved these changes Jan 7, 2018

View reviewed changes

cjihrig approved these changes Jan 7, 2018

View reviewed changes

joyeecheung reviewed Jan 9, 2018

View reviewed changes

addaleax force-pushed the no-has-write-queue branch from c394888 to 255630c Compare January 9, 2018 19:12

addaleax added the wip Issues and PRs that are still a work in progress. label Jan 9, 2018

addaleax added 3 commits January 10, 2018 20:42

src: remove HasWriteQueue()

fad18b4

Tests are passing without it, and this otherwise makes the code harder to reason about because the `async` flag on the write request object would not be set even though the callback would still be pending.

src: use DoTryWrite() for not-all-Buffer writev()s too

72ff460

addaleax force-pushed the no-has-write-queue branch from 255630c to 72ff460 Compare January 10, 2018 19:47

addaleax removed the wip Issues and PRs that are still a work in progress. label Jan 10, 2018

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

addaleax closed this Jan 14, 2018

addaleax deleted the no-has-write-queue branch January 14, 2018 13:47

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2018

addaleax added a commit that referenced this pull request Feb 26, 2018

src: use DoTryWrite() for not-all-Buffer writev()s too

c8741ba

PR-URL: #18019 Reviewed-By: James M Snell <[email protected]>

addaleax added a commit that referenced this pull request Feb 26, 2018

src: use DoTryWrite() for not-all-Buffer writev()s too

f607577

PR-URL: #18019 Reviewed-By: James M Snell <[email protected]>

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins added backported-to-v8.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Aug 17, 2018

MylesBorins pushed a commit that referenced this pull request Aug 17, 2018

src: use DoTryWrite() for not-all-Buffer writev()s too

cbe9239

PR-URL: #18019 Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

vtjnash mentioned this pull request Jan 13, 2020

win: pipe UV_HANDLE_EMULATE_IOCP implementation broken libuv/libuv#629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: refactor HasWriteQueue() away #18019

src: refactor HasWriteQueue() away #18019

addaleax commented Jan 6, 2018

addaleax commented Jan 6, 2018 •

edited

Loading

apapirovski left a comment •

edited

Loading

addaleax commented Jan 6, 2018

apapirovski commented Jan 6, 2018

joyeecheung Jan 9, 2018

addaleax Jan 9, 2018

addaleax commented Jan 10, 2018 •

edited

Loading

jasnell commented Jan 10, 2018

addaleax commented Jan 14, 2018

codebytere commented Aug 2, 2018 •

edited

Loading

MylesBorins commented Aug 17, 2018

src: refactor HasWriteQueue() away #18019

src: refactor HasWriteQueue() away #18019

Conversation

addaleax commented Jan 6, 2018

Checklist

Affected core subsystem(s)

addaleax commented Jan 6, 2018 • edited Loading

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

addaleax commented Jan 6, 2018

apapirovski commented Jan 6, 2018

joyeecheung Jan 9, 2018

Choose a reason for hiding this comment

addaleax Jan 9, 2018

Choose a reason for hiding this comment

addaleax commented Jan 10, 2018 • edited Loading

jasnell commented Jan 10, 2018

addaleax commented Jan 14, 2018

codebytere commented Aug 2, 2018 • edited Loading

MylesBorins commented Aug 17, 2018

addaleax commented Jan 6, 2018 •

edited

Loading

apapirovski left a comment •

edited

Loading

addaleax commented Jan 10, 2018 •

edited

Loading

codebytere commented Aug 2, 2018 •

edited

Loading