Skip to content

Commit

Permalink
fix(runtime): runtime crashes with EAGAIN trying to read from STDIN (#…
Browse files Browse the repository at this point in the history
…1143)

When using node >= 13.2, attempts to synchronously read from STDIN often
result in `EAGAIN` being raised. This is due to the STDIN file
descriptor being opened with `O_NONBLOCK`.

This introduces a temporary workaround that catches the EAGAIN exception
and re-attempts reading from STDIN "immediately". While this is not the
ideal solution, it can be done right away and appears to have minimal
impact on the overall performance of applications.

A longer term fix is to move away from synchronous IO operations, but
this is a significantly mroe involved fix and migth be a while before
this can be delivered. The long term solution is tracked in #1142.

Related to aws/aws-cdk#5187
  • Loading branch information
RomainMuller committed Dec 19, 2019
1 parent 674181e commit e3502ed
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 14 deletions.
36 changes: 24 additions & 12 deletions packages/@jsii/runtime/lib/sync-stdio.ts
Expand Up @@ -22,21 +22,33 @@ export class SyncStdio {
return this.inputQueue.shift();
}

const buff = Buffer.alloc(INPUT_BUFFER_SIZE);
const read = fs.readSync(STDIN_FD, buff, 0, buff.length, null);
try {
const buff = Buffer.alloc(INPUT_BUFFER_SIZE);
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 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;
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/@jsii/runtime/package.json
Expand Up @@ -64,8 +64,8 @@
],
"coverageThreshold": {
"global": {
"branches": 44,
"statements": 57
"branches": 43,
"statements": 56
}
},
"errorOnDeprecated": true,
Expand Down

0 comments on commit e3502ed

Please sign in to comment.