Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console: use synchronous write when the process is piped #25638

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion lib/internal/console/global.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,25 @@ for (const prop of Reflect.ownKeys(Console.prototype)) {
Reflect.defineProperty(globalConsole, prop, desc);
}

globalConsole[kBindStreamsLazy](process);
function makeSync(stream) {
// This function is only called twice, we are not reusing the require call.
const SyncWriteStream = require('internal/fs/sync_write_stream');

if (stream.isTTY || stream instanceof SyncWriteStream) {
return stream;
} else if (stream.fd >= 0) {
return new SyncWriteStream(stream.fd);
} else {
// We cannot do much more, the stream will be async.
// TODO(mcollina) verify if such a case is possible
return stream;
}
}

globalConsole[kBindStreamsLazy]({
get stdout() { return makeSync(process.stdout); },
get stderr() { return makeSync(process.stderr); }
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all console instances behave the same? I would move this into the kBindStreamsEager and kBindStreamsLazy functions in the console constructor file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely. We might also move this whole sync logic into the console, and adding capability there to manage the fd directly.

I’ll wait and see if we can settle on this approach before making any changes.

globalConsole[kBindProperties](true, 'auto');

// This is a legacy feature - the Console constructor is exposed on
Expand Down
16 changes: 15 additions & 1 deletion lib/internal/fs/sync_write_stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,21 @@ Object.setPrototypeOf(SyncWriteStream.prototype, Writable.prototype);
Object.setPrototypeOf(SyncWriteStream, Writable);

SyncWriteStream.prototype._write = function(chunk, encoding, cb) {
writeSync(this.fd, chunk, 0, chunk.length);
while (true) {
try {
const n = writeSync(this.fd, chunk, 0, chunk.length);
if (n !== chunk.length) {
chunk = chunk.slice(0, n);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the last bytes written first? AFAIK this should be chunk = chunk.slice(n)? Maybe just verify that in the test as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina PTAL. This seems wrong to me.

} else {
break;
}
} catch (err) {
if (err.code !== 'EAGAIN') {
cb(err);
break;
}
}
}
cb();
return true;
};
Expand Down
34 changes: 34 additions & 0 deletions test/parallel/test-console-block-when-piped.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
const KB = 1024;
const MB = KB * KB;
const expected = KB * MB + MB; // 1GB + 1MB of newlines

// console.log must synchronously write to stdout if
// if the process is piped to. The expected behavior is that
// console.log will block the main thread if the consumer
// cannot keep up.
// See https://github.com/nodejs/node/issues/24992 for more
// details.

if (process.argv[2] === 'child') {
const data = Buffer.alloc(KB).fill('x').toString();
for (let i = 0; i < MB; i++)
console.log(data);
process.exit(0);
} else {
const child = cp.spawn(process.execPath, [__filename, 'child'], {
stdio: ['pipe', 'pipe', 'inherit']
});
let count = 0;
child.stdout.on('data', (c) => count += c.length);
child.stdout.on('end', common.mustCall(() => {
assert.strictEqual(count, expected);
}));
child.on('exit', common.mustCall((code) => {
assert.strictEqual(code, 0);
}));
}