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.

Sign up for GitHub

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

Jump to bottom

fix(runtime): runtime crashes with EAGAIN trying to read from STDIN #1143

Merged
merged 1 commit into from
Dec 19, 2019
Merged

fix(runtime): runtime crashes with EAGAIN trying to read from STDIN #1143

merged 1 commit into from
Dec 19, 2019

Conversation

RomainMuller
Copy link
Contributor

When using node >= 13.2, attempts to synchronously read from STDIN often
result in EAGAIN being raised. This is due to the STDIN file
descriptor being opened with O_NONBLOCK.

This introduces a temporary workaround that catches the EAGAIN exception
and re-attempts reading from STDIN "immediately". While this is not the
ideal solution, it can be done right away and appears to have minimal
impact on the overall performance of applications.

A longer term fix is to move away from synchronous IO operations, but
this is a significantly mroe involved fix and migth be a while before
this can be delivered. The long term solution is tracked in #1142.

Related to aws/aws-cdk#5187

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

@RomainMuller RomainMuller requested review from MrArnoldPalmer and a team as code owners December 19, 2019 09:15
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 19, 2019

Do you have a reference to the Node issue tracker for this issue somewhere? Do they consider it a feature?

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 19, 2019

BTW we can probably switch stdin back to blocking.

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 19, 2019

Can we add the tiniest of blocking sleep? Probably doesn't exist eh?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@RomainMuller
Copy link
Contributor Author

RomainMuller commented Dec 19, 2019 •
edited
Loading

node (or libuv?) has had STDIN open in O_NONBLOCK since some point in 0.x (virtually forever), so the code has always been susceptible to EAGAIN. Generally speaking, the behavior around this is different in Windows versus *NIXes (that makes things even funnier, doesn't it?). This StackOverflow response has a pretty good summary of the situation.

As a matter of fact, the process.stdin object is a net.Socket in the failing executions, and that class is fully asynchronous in the node standard library.

I also could not find a way to access fcntl from js-land, which would be required to flip the O_NONBLOCK flag around for safe synchronous operations.

Regarding introducing of a sleep instruction... Well as you have it... there isn't a synchronous sleep possibly in node, short of (synchronously) shelling out to sleep.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@RomainMuller
fix(runtime): runtime crashes with EAGAIN trying to read from STDIN
aa5bc77 
When using node >= 13.2, attempts to synchronously read from STDIN often
result in `EAGAIN` being raised. This is due to the STDIN file
descriptor being opened with `O_NONBLOCK`.

This introduces a temporary workaround that catches the EAGAIN exception
and re-attempts reading from STDIN "immediately". While this is not the
ideal solution, it can be done right away and appears to have minimal
impact on the overall performance of applications.

A longer term fix is to move away from synchronous IO operations, but
this is a significantly mroe involved fix and migth be a while before
this can be delivered. The long term solution is tracked in #1142.

Related to aws/aws-cdk#5187
@RomainMuller RomainMuller force-pushed the rmuller/hack-around-node-13.2+-issue branch from 5ab5034 to aa5bc77 Compare December 19, 2019 13:34
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@RomainMuller RomainMuller merged commit e3502ed into master Dec 19, 2019
@RomainMuller RomainMuller deleted the rmuller/hack-around-node-13.2+-issue branch December 19, 2019 14:17
@MrArnoldPalmer MrArnoldPalmer mentioned this pull request Jan 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr approved these changes

@MrArnoldPalmer MrArnoldPalmer Awaiting requested review from MrArnoldPalmer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RomainMuller @rix0rrr @aws-cdk-automation