-
Notifications
You must be signed in to change notification settings - Fork 245
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
fix(@jsii/runtime): "maximum call stack size exceeded" in SyncStdio.readLine #1717
Changes from 3 commits
599fc52
ecd05a6
fb4216c
fc5bc91
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 |
---|---|---|
|
@@ -18,45 +18,41 @@ export class SyncStdio { | |
} | ||
|
||
public readLine(): string | undefined { | ||
if (this.inputQueue.length > 0) { | ||
return this.inputQueue.shift(); | ||
} | ||
|
||
try { | ||
const buff = Buffer.alloc(INPUT_BUFFER_SIZE); | ||
const read = fs.readSync(STDIN_FD, buff, 0, buff.length, null); | ||
const buff = Buffer.alloc(INPUT_BUFFER_SIZE); | ||
while (this.inputQueue.length === 0) { | ||
try { | ||
const read = fs.readSync(STDIN_FD, buff, 0, buff.length, null); | ||
|
||
if (read === 0) { | ||
return undefined; | ||
} | ||
if (read === 0) { | ||
return undefined; | ||
} | ||
|
||
const str = buff.slice(0, read).toString(); | ||
const str = buff.slice(0, read).toString(); | ||
|
||
for (const ch of str) { | ||
if (ch === '\n') { | ||
this.inputQueue.push(this.currentLine); | ||
this.currentLine = ''; | ||
} else { | ||
this.currentLine += ch; | ||
for (const ch of str) { | ||
if (ch === '\n') { | ||
this.inputQueue.push(this.currentLine); | ||
this.currentLine = ''; | ||
} else { | ||
this.currentLine += ch; | ||
} | ||
} | ||
} catch (e) { | ||
// HACK: node may set O_NONBLOCK on it's STDIN depending on what kind of input it is made | ||
// of (see https://github.com/nodejs/help/issues/2663). When STDIN has O_NONBLOCK, calls may | ||
// result in EAGAIN. In such cases, the call should be retried until it succeeds. This kind | ||
// of polling will result in horrible CPU thrashing, but there does not seem to be a way to | ||
// force a O_SYNC access to STDIN in a reliable way within node. | ||
// In order to make this stop we need to either stop depending on synchronous reads, or to | ||
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.
That would effectively mean callbacks would stop working, right? 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. Why no. This means asynchronous user-code suddenly would throw everything off. Pure-async kernel would work okay if we could guarantee client code NEVER does anything asynchronous by itself. |
||
// provision our own communication channel that can reliably be synchronous. This work is | ||
// "tracked" at https://github.com/aws/aws-cdk/issues/5187 | ||
if (e.code !== 'EAGAIN') { | ||
throw e; | ||
} | ||
} | ||
} catch (e) { | ||
// HACK: node opens STDIN with O_NONBLOCK, meaning attempts to synchrounously read from it may | ||
// result in EAGAIN. In such cases, the call should be retried until it succeeds. This kind of | ||
// polling may be terribly inefficient, and the "correct" way to address this is to stop | ||
// relying on synchronous reads from STDIN. This is however a non-trivial endeavor, and the | ||
// current state of things is very much broken in node >= 13.2, as can be see in | ||
// https://github.com/aws/aws-cdk/issues/5187 | ||
if (e.code !== 'EAGAIN') { | ||
throw e; | ||
} | ||
} | ||
|
||
const next = this.inputQueue.shift(); | ||
if (next == null) { | ||
return this.readLine(); | ||
} | ||
|
||
return next; | ||
} | ||
|
||
|
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.
We control both sides of the pipe. In what conditions will node use non blocking? Is this something we can configure on the runtime side?
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.
According to the linked thread, something as simple as calling
console.log()
may switchprocess.stdin
to non-blocking mode.