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

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.

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.

[sleep]: https://www.npmjs.com/package/sleep

.github/workflows/main.yml

@@ -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

packages/@jsii/runtime/lib/program.ts

@@ -1,3 +1,5 @@
+import * as process from 'process';
+
 import * as packageInfo from '../package.json';
 import { KernelHost } from './host';
 import { InputOutput } from './in-out';

packages/@jsii/runtime/lib/sleep.ts

@@ -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, 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);
+}

packages/@jsii/runtime/lib/sync-stdio.ts

@@ -1,14 +1,20 @@
 import * as fs from 'fs';
 
-const STDIN_FD = 0;
-const STDOUT_FD = 1;
-const STDERR_FD = 2;
-const INPUT_BUFFER_SIZE = 1024 * 1024; // not related to max line length
+import { sleep } from './sleep';
 
-const EMPTY_BUFFER = Buffer.alloc(0);
+// Note: the `process.std{in,out,err}.fd` is not part of the `@types/node` declarations, because
+// those cannot model how those fields are guaranteed to be absent within the context of worker
+// threads. The should be present here, but since we must resort to `as any`, we take the extra
+// precaution of defaulting if those values are not present.
+
+const STDIN_FD: number = (process.stdin as any).fd ?? 0;
+const STDOUT_FD: number = (process.stdout as any).fd ?? 1;
+const STDERR_FD: number = (process.stderr as any).fd ?? 2;
+
+const INPUT_BUFFER_SIZE = 1_048_576; // 1MiB (aka: 1024 * 1024), not related to max line length
 
 export class SyncStdio {
-  private bufferedData = EMPTY_BUFFER;
+  private bufferedData = Buffer.alloc(0);
 
   public writeErrorLine(line: string) {
     this.writeBuffer(Buffer.from(`${line}\n`), STDERR_FD);
@@ -21,28 +27,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);
+      const read = readSync(STDIN_FD, buff, 0, buff.length);
 
-        if (read === 0) {
-          return undefined;
-        }
-
-        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');
@@ -61,6 +53,64 @@ export class SyncStdio {
         if (e.code !== 'EAGAIN') {
           throw e;
         }
+        sleep(50 /*ms*/);
+      }
+    }
+  }
+}
+
+/**
+ * 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 {
+  // We are using a `while (true)` here to avoid recursively re-entering the function, in order to
+  // avoid thrashing memory with stack frames, when we are more likely to face `EAGAIN` on systems
+  // with low resources (near memory limit and/or high load).
+  //
+  // 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 could result in horrible CPU thrashing: while we can sleep between two
+        // attempts, sleeping too much would slow everything to a crawl, and not enough would cause
+        // significant wasting of CPU cycles.
+        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;
       }
     }
   }

packages/@jsii/runtime/test/playback.test.ts

@@ -41,17 +41,19 @@ function createRecords(): string {
     [
       ...process.execArgv,
       require.resolve('jest/bin/jest'),
-      '--coverage=false',
+      '--no-coverage',
+      '--runInBand',
       'test/kernel.test.ts',
     ],
     {
-      env: { ...process.env, JSII_RECORD: records, JSII_NOSTACK: '1' },
-      stdio: ['inherit', 'pipe', 'pipe'],
       cwd: path.resolve(
         require.resolve('@jsii/kernel/test/kernel.test.js'),
         '..',
         '..',
       ),
+      env: { ...process.env, JSII_RECORD: records, JSII_NOSTACK: '1' },
+      stdio: ['inherit', 'pipe', 'pipe'],
+      timeout: 60_000, // 1 minute
     },
   );