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

Stop relying on synchronous reads from STDIN #1142

Closed
3 of 4 tasks
RomainMuller opened this issue Dec 19, 2019 · 5 comments
Closed
3 of 4 tasks

Stop relying on synchronous reads from STDIN #1142

RomainMuller opened this issue Dec 19, 2019 · 5 comments
Labels
bug This issue is a bug. closed-for-staleness effort/large Large work item – several weeks of effort module/runtime Issues affecting the `jsii-runtime` p2

Comments

@RomainMuller
Copy link
Contributor

🐛 Bug Report

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)

What is the problem?

node opens it's STDIN file descriptor in O_NONBLOCK mode, making attempts to synchronously read unsafe. Starting with node 13.2, there has been a significant increase in the incidence of hitting EAGAIN in this process. Synchronous mitigations are tacky at best, and the proper way to fix is to move away from synchronous IO.

See aws/aws-cdk#5187

@RomainMuller RomainMuller added bug This issue is a bug. module/runtime Issues affecting the `jsii-runtime` p2 labels Dec 19, 2019
RomainMuller added a commit that referenced this issue Dec 19, 2019
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
@eladb
Copy link
Contributor

eladb commented Dec 19, 2019

I don't think it will be possible to move away from synchronous reads since you need to invoke host callbacks during synchronous javascript function calls, so you need to block the javascript execution.

@eladb
Copy link
Contributor

eladb commented Dec 19, 2019

Can you also provide a reference to the node.js documentation that says that stdin is opened with O_NONBLOCK? From joyent/libuv@1988f5e is seems like it's actually opened without O_NONBLOCK

RomainMuller added a commit that referenced this issue Dec 19, 2019
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 added a commit that referenced this issue Dec 19, 2019
…1143)

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

Can you also provide a reference to the node.js documentation that says that stdin is opened with O_NONBLOCK? From joyent/libuv@1988f5e is seems like it's actually opened without O_NONBLOCK

The code you are pointing at is the code that initializes the STDIN/OUT for child processes, which was introduced around nodejs/node-v0.x-archive#3994, where people very specifically call out that, unlike node, most other programs don't have a non-blocking STDIN and hit problems when they do.

@RomainMuller
Copy link
Contributor Author

That being said, I think you're correct about it not being possible to make this process synchronous (or it'd require the application's code performs NO asynchronous operation ever).

@github-actions
Copy link
Contributor

This issue has not received any attention in 2 years. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. closed-for-staleness effort/large Large work item – several weeks of effort module/runtime Issues affecting the `jsii-runtime` p2
Projects
None yet
Development

No branches or pull requests

2 participants