-
Notifications
You must be signed in to change notification settings - Fork 246
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
Changes from 2 commits
221edff
2d6b7e1
cf7a039
a16a5c2
6710960
20a892f
7f666ae
963b1af
4ea26d9
f7f189e
10f8498
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 |
---|---|---|
@@ -1,3 +1,5 @@ | ||
import * as process from 'process'; | ||
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. Added as part of my rumblings here... |
||
|
||
import * as packageInfo from '../package.json'; | ||
import { KernelHost } from './host'; | ||
import { InputOutput } from './in-out'; | ||
|
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. | ||||||
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.
Suggested change
|
||||||
* | ||||||
* @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); | ||||||
} |
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); | ||
|
@@ -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'); | ||
|
@@ -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
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. I used a 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. 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 | ||
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. I see this was copy/pasted; is it still relevant? The referenced issue is closed. Do we have an active/open issue, and/or |
||
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; | ||
} | ||
} | ||
} | ||
} |
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.
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.