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

RomainMuller · 2020-11-09T09:56:57Z

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:
using Atomics.wait enables syncrhonous sleep to be done in node
without having to ship a native module.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

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

RomainMuller · 2020-11-09T15:01:00Z

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

@@ -1,3 +1,5 @@
+import * as process from 'process';


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.

RomainMuller · 2020-11-09T15:01:57Z

.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


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.

RomainMuller · 2020-11-09T15:06:12Z

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

+  // eslint-disable-next-line no-constant-condition
+  while (true) {


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).

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

njlynch

All of my comments are about comments. :)

Also, are there any tests for this section of the code?

njlynch · 2020-11-09T17:09:53Z

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

+        // 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


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?

njlynch · 2020-11-09T17:11:41Z

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


-const EMPTY_BUFFER = Buffer.alloc(0);
+const STDIN_FD = (process.stdin as any).fd ?? 0;


When will these (not) be set? Might deserve a comment.

njlynch · 2020-11-09T17:12:26Z

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

+const array = new Int32Array(new SharedArrayBuffer(4));
+
+/**
+ * Sleeps for a set amount of time, syncrhonously.


Suggested change

* Sleeps for a set amount of time, syncrhonously.

* Sleeps for a set amount of time, synchronously.

njlynch · 2020-11-09T17:13:58Z

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

+  // eslint-disable-next-line no-constant-condition
+  while (true) {


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

mergify · 2020-11-10T10:26:42Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-11-10T10:27:20Z

Merging (with squash)...

aws-cdk-automation · 2020-11-10T12:11:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-5lHf64IXfvmr
Commit ID: 10f8498
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-11-10T13:07:53Z

Merging (with squash)...

RomainMuller added effort/small Small work item – less than a day of effort os/windows Related specifically to Windows behavior labels Nov 9, 2020

RomainMuller requested a review from a team November 9, 2020 09:56

RomainMuller self-assigned this Nov 9, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 9, 2020

RomainMuller marked this pull request as draft November 9, 2020 13:53

RomainMuller force-pushed the rmuller/windows-eof branch from c4b7c50 to 221edff Compare November 9, 2020 14:58

RomainMuller changed the title ~~chore(ci): test more combinations with Windows~~ fix(runtime): "Error: EOF: end of file, read" on Windows Nov 9, 2020

RomainMuller marked this pull request as ready for review November 9, 2020 14:59

RomainMuller commented Nov 9, 2020

View reviewed changes

typo

2d6b7e1

RomainMuller commented Nov 9, 2020

View reviewed changes

RomainMuller mentioned this pull request Nov 9, 2020

fs.readSync throws EOF Error instead of returning 0 (Windows, if STDIN is a pipe) nodejs/node#35997

Open

RomainMuller added 6 commits November 9, 2020 16:19

try to tweak re-opening flags

cf7a039

also wait on EAGAIN during write

a16a5c2

re-work the STDIO re-opening for safety

6710960

improve buffer handling, sligthly

20a892f

tuned up comment

7f666ae

stop trying to re-open stdios it does not work

963b1af

njlynch reviewed Nov 9, 2020

View reviewed changes

comments from @njlynch

4ea26d9

njlynch approved these changes Nov 10, 2020

View reviewed changes

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Nov 10, 2020

RomainMuller and others added 2 commits November 10, 2020 12:42

tweaks

f7f189e

Merge branch 'main' into rmuller/windows-eof

10f8498

RomainMuller merged commit 1453ed3 into main Nov 10, 2020

RomainMuller deleted the rmuller/windows-eof branch November 10, 2020 14:47

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Nov 10, 2020

RomainMuller mentioned this pull request Nov 30, 2020

release 1.15.0 makes cdk synth radically slow #2284

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RomainMuller commented Nov 9, 2020 •

edited

RomainMuller Nov 9, 2020

RomainMuller Nov 9, 2020

RomainMuller Nov 9, 2020

njlynch Nov 9, 2020

njlynch left a comment

njlynch Nov 9, 2020

njlynch Nov 9, 2020

njlynch Nov 9, 2020

njlynch Nov 9, 2020

mergify bot commented Nov 10, 2020

mergify bot commented Nov 10, 2020

aws-cdk-automation commented Nov 10, 2020

mergify bot commented Nov 10, 2020

		// eslint-disable-next-line no-constant-condition
		while (true) {


		const EMPTY_BUFFER = Buffer.alloc(0);
		const STDIN_FD = (process.stdin as any).fd ?? 0;

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

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

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

Conversation

RomainMuller commented Nov 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Nov 10, 2020

mergify bot commented Nov 10, 2020

aws-cdk-automation commented Nov 10, 2020

AWS CodeBuild CI Report

mergify bot commented Nov 10, 2020

RomainMuller commented Nov 9, 2020 •

edited