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

fix(runtime): "Error: EOF: end of file, read" on Windows #2238

Merged
merged 11 commits into from
Nov 10, 2020
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly related to this PR, but this can be helpful in the future. This is kinda trivial, too. This got added as I was trying to find a way to reliably cause an EOF to happen on Windows in CI/CD. Spoiler: I did not succeed, for some reason.

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';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as part of my rumblings here... process.env is used here, and I kinda like to standardize on importing this even though it's implicit.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Sleeps for a set amount of time, syncrhonously.
* Sleeps for a set amount of time, synchronously.

*
* @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 involving `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) {
Comment on lines +86 to +87
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used a while loop instead of recursively re-entering, as this avoids stacking up in case EAGAIN happens a lot, which I imagine might be somewhat frequent on resource-constrained systems (where we'd likely also have less memory available for wasting on stack frames).

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a nice comment to have in source, rather than just on the PR.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this was copy/pasted; is it still relevant? The referenced issue is closed. Do we have an active/open issue, and/or jsii issue to "track" this?

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;
}
}
}
}