-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the last bytes written first? AFAIK this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; | ||
|
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); | ||
})); | ||
} |
There was a problem hiding this comment.
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 thekBindStreamsEager
andkBindStreamsLazy
functions in the console constructor file.There was a problem hiding this comment.
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.