Skip to content

Commit

Permalink
fix(runtime): "Error: EOF: end of file, read" on Windows
Browse files Browse the repository at this point in the history
NodeJS makes it unsafe to perform syncrhonous (aka blocking) operations
on file descriptors 0 through 2 (STDIN, STDOUT, STDERR), as those are
set to `O_NONBLOCK` by `libuv`. This is particularly problematic on
Windows where there is no easy way to re-open those in blocking mode as
can be done on most UNIX platforms today using `/dev/std{in,out,err}`
pseudo-files.

Consequently, we must handle some quirks in cases where blocking
attempts result in conditions typical of non-blocking access: `EAGAIN`
errors must be caught and retried, and `EOF` errors must be "ignored".

This PR adds the necessary code to correctly handle the `EOF` error, and
adds a 50ms sleep before re-trying a `EAGAIN` error that is achieved
thanks to a neat trick suggested by the [`sleep` npm package][sleep]:
using `Atomics.wait` enables syncrhonous sleep to be done in `node`
without having to ship a native module.

Additionally, this PRs re-opens `STDIN`, `STDOUT` and `STDERR` when the
`/dev/std{in,out,err}` files are present, reducing the incidence of this
problem on most UNIX platfoms.

[sleep]: https://www.npmjs.com/package/sleep
  • Loading branch information
RomainMuller committed Nov 9, 2020
1 parent 32774ca commit 221edff
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 23 deletions.
1 change: 1 addition & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ jobs:
python: '3.6'
# Test alternate .NETs
- java: '8'
# Current release info can be found at https://dotnet.microsoft.com/download/dotnet/5.0
dotnet: '5.0.100-rc.2.20479.15' # Pre-release matching requires exact version for now
node: '10'
os: ubuntu-latest
Expand Down
2 changes: 2 additions & 0 deletions packages/@jsii/runtime/lib/program.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import * as process from 'process';

import * as packageInfo from '../package.json';
import { KernelHost } from './host';
import { InputOutput } from './in-out';
Expand Down
14 changes: 14 additions & 0 deletions packages/@jsii/runtime/lib/sleep.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// We need a shared buffer array for the purpose of this exercise.
const array = new Int32Array(new SharedArrayBuffer(4));

/**
* Sleeps for a set amount of time, syncrhonously.
*
* @param time the amount of time to wait, in milliseconds.
*/
export function sleep(time: number): void {
// `Atomics.wait` will block for `time` milliseconds if `array[0]` still has
// value `0` (which it will, since we just initialized it to that). The return
// value is irrelevant for our business here.
Atomics.wait(array, 0, 0, time);
}
101 changes: 78 additions & 23 deletions packages/@jsii/runtime/lib/sync-stdio.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
import * as fs from 'fs';

const STDIN_FD = 0;
const STDOUT_FD = 1;
const STDERR_FD = 2;
import { sleep } from './sleep';

// When possible (i.e: we are on a UNIX system where `/dev/std{in,out,err}` pseudo-files exist), we
// will re-open those streams in synchronous mode, because `node` might otherwise turn those to
// `O_NONBLOCK` under our feet, causing vast inefficiencies later on (as we must busy-poll around
// `EAGAIN` errors). Notably, Windows does not offer the `/dev/std{in,out,err}` interfaces, so we
// must still be able to handle unexpected non-blocking-ness.
const STDIN_FD = fs.existsSync('/dev/stdin')
? fs.openSync('/dev/stdin', fs.constants.O_RDONLY | fs.constants.O_SYNC)
: 0;
const STDOUT_FD = fs.existsSync('/dev/stdout')
? fs.openSync('/dev/stdout', fs.constants.O_WRONLY | fs.constants.O_SYNC)
: 1;
const STDERR_FD = fs.existsSync('/dev/stderr')
? fs.openSync('/dev/stderr', fs.constants.O_WRONLY | fs.constants.O_SYNC)
: 2;

const INPUT_BUFFER_SIZE = 1024 * 1024; // not related to max line length

const EMPTY_BUFFER = Buffer.alloc(0);
Expand All @@ -21,28 +35,14 @@ export class SyncStdio {
public readLine(): string | undefined {
const buff = Buffer.alloc(INPUT_BUFFER_SIZE);
while (!this.bufferedData.includes('\n', 0, 'utf-8')) {
try {
const read = fs.readSync(STDIN_FD, buff, 0, buff.length, null);

if (read === 0) {
return undefined;
}
const read = readSync(STDIN_FD, buff, 0, buff.length);

const newData = buff.slice(0, read);
this.bufferedData = Buffer.concat([this.bufferedData, newData]);
} 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
// 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;
}
if (read === 0) {
return undefined;
}

const newData = buff.slice(0, read);
this.bufferedData = Buffer.concat([this.bufferedData, newData]);
}

const newLinePos = this.bufferedData.indexOf('\n', 0, 'utf-8');
Expand All @@ -65,3 +65,58 @@ export class SyncStdio {
}
}
}

/**
* A patched up implementation of `fs.readSync` that works around the quirks associated with
* synchronous I/O invoolving `STDIN`, `STDOUT` and `STDERR` on NodeJS, where this is all expected
* to be asynchronous and hence has some "interesting" behavior in certain particular cases.
*
* @param fd the file descriptor to read from (typically 0 / STDIN)
* @param buffer the buffer in which to place the read data
* @param offset the offset in the buffer at which to place the read data
* @param length the number of bytes to be read
* @param position where to begin reading from the file (defaults to the current read location)
*
* @returns the amount of bytes read, or `0` if EOF has been reached.
*/
function readSync(
fd: number,
buffer: Buffer,
offset: number,
length: number,
position: number | null = null,
): number {
// eslint-disable-next-line no-constant-condition
while (true) {
try {
return fs.readSync(fd, buffer, offset, length, position);
} catch (error) {
switch (error.code) {
// 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
// provision our own communication channel that can reliably be synchronous. This work is
// "tracked" at https://github.com/aws/aws-cdk/issues/5187
case 'EAGAIN':
// Keep trying until it no longer says EAGAIN. We'll be waiting a little before retrying
// in order to avoid thrashing the CPU like there is no tomorrow. This is not entirely
// ideal, but it has to do.
sleep(50 /*ms*/);
break;

// HACK: in Windows, when STDIN (aka FD#0) is wired to a socket (as is the case when started
// as a subprocess with piped IO), `fs.readSync` will throw "Error: EOF: end of file, read"
// instead of returning 0 (which is what the documentation suggests it would do). This is
// currently believed to be a bug in `node`: https://github.com/nodejs/node/issues/35997
case 'EOF':
return 0;

default:
throw error;
}
}
}
}

0 comments on commit 221edff

Please sign in to comment.