Skip to content

Commit

Permalink
process: use more direct sync I/O for stdio
Browse files Browse the repository at this point in the history
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 <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax authored and MylesBorins committed Aug 17, 2018
1 parent 69efa9f commit 2ec981b
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 4 deletions.
24 changes: 24 additions & 0 deletions lib/internal/net.js
@@ -1,5 +1,8 @@
'use strict';

const Buffer = require('buffer').Buffer;
const { writeBuffer } = process.binding('fs');

// Check that the port number is not NaN when coerced to a number,
// is an integer and that it falls within the legal range of port numbers.
function isLegalPort(port) {
Expand All @@ -9,7 +12,28 @@ function isLegalPort(port) {
return +port === (+port >>> 0) && port <= 0xFFFF;
}

function makeSyncWrite(fd) {
return function(chunk, enc, cb) {
if (enc !== 'buffer')
chunk = Buffer.from(chunk, enc);

this._bytesDispatched += chunk.length;

try {
writeBuffer(fd, chunk, 0, chunk.length, null);
} catch (ex) {
// Legacy: net writes have .code === .errno, whereas writeBuffer gives the
// raw errno number in .errno.
if (typeof ex.code === 'string')
ex.errno = ex.code;
return cb(ex);
}
cb();
};
}

module.exports = {
isLegalPort,
makeSyncWrite,
normalizedArgsSymbol: Symbol('normalizedArgs')
};
16 changes: 12 additions & 4 deletions lib/net.js
Expand Up @@ -26,7 +26,11 @@ const stream = require('stream');
const timers = require('timers');
const util = require('util');
const internalUtil = require('internal/util');
const { isLegalPort, normalizedArgsSymbol } = require('internal/net');
const {
isLegalPort,
normalizedArgsSymbol,
makeSyncWrite
} = require('internal/net');
const assert = require('assert');
const cares = process.binding('cares_wrap');
const uv = process.binding('uv');
Expand Down Expand Up @@ -206,20 +210,24 @@ function Socket(options) {
this._handle = options.handle; // private
this[async_id_symbol] = getNewAsyncId(this._handle);
} else if (options.fd !== undefined) {
this._handle = createHandle(options.fd, false);
this._handle.open(options.fd);
const fd = options.fd;
this._handle = createHandle(fd, false);
this._handle.open(fd);
this[async_id_symbol] = this._handle.getAsyncId();
// options.fd can be string (since it is user-defined),
// so changing this to === would be semver-major
// See: https://github.com/nodejs/node/pull/11513
// eslint-disable-next-line eqeqeq
if ((options.fd == 1 || options.fd == 2) &&
if ((fd == 1 || fd == 2) &&
(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);
}
this.readable = options.readable !== false;
this.writable = options.writable !== false;
Expand Down
3 changes: 3 additions & 0 deletions lib/tty.js
Expand Up @@ -24,6 +24,7 @@
const { inherits, _extend } = require('util');
const net = require('net');
const { TTY, isTTY } = process.binding('tty_wrap');
const { makeSyncWrite } = require('internal/net');
const errors = require('internal/errors');
const readline = require('readline');

Expand Down Expand Up @@ -77,6 +78,8 @@ function WriteStream(fd) {
// even though it was originally intended to change in v1.0.2 (Libuv 1.2.1).
// Ref: https://github.com/nodejs/node/pull/1771#issuecomment-119351671
this._handle.setBlocking(true);
this._writev = null;
this._write = makeSyncWrite(fd);

var winSize = new Array(2);
var err = this._handle.getWindowSize(winSize);
Expand Down

0 comments on commit 2ec981b

Please sign in to comment.