fix(@jsii/runtime): "maximum call stack size exceeded" in SyncStdio.readLine #1717
Conversation
…eadLine When using node `>= 12.17`, `EAGAIN` errors consistently occur when trying to read from `stdin` when there is no available data. The retry mechanism for this was to recursively call back `SyncStdio.readLine`, which could evtnually lead to a "Maximum call stack size exceeded" error to occur (possibly hidden from the consumer, and later causing a "Stream closed" error). This changes how the retry mechanism works so it operates in a `while` loop instead of making a recursive call, completely avoiding to run into the growing stack issue. Fixes aws/aws-cdk#8288 Fixes aws/aws-cdk#5187
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| } | ||
| } | ||
| } catch (e) { | ||
| // HACK: node may set O_NONBLOCK on it's STDIN depending on what kind of input it is made |
There was a problem hiding this comment.
We control both sides of the pipe. In what conditions will node use non blocking? Is this something we can configure on the runtime side?
There was a problem hiding this comment.
According to the linked thread, something as simple as calling console.log() may switch process.stdin to non-blocking mode.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| } | ||
| } | ||
| } catch (e) { | ||
| // HACK: node may set O_NONBLOCK on it's STDIN depending on what kind of input it is made |
There was a problem hiding this comment.
According to the linked thread, something as simple as calling console.log() may switch process.stdin to non-blocking mode.
| // 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 |
There was a problem hiding this comment.
stop depending on synchronous reads
That would effectively mean callbacks would stop working, right?
There was a problem hiding this comment.
Why no. This means asynchronous user-code suddenly would throw everything off. Pure-async kernel would work okay if we could guarantee client code NEVER does anything asynchronous by itself.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Merging (with squash)... |
When using node
>= 12.17,EAGAINerrors consistently occur whentrying to read from
stdinwhen there is no available data. The retrymechanism for this was to recursively call back
SyncStdio.readLine,which could evtnually lead to a "Maximum call stack size exceeded" error
to occur (possibly hidden from the consumer, and later causing a "Stream
closed" error).
This changes how the retry mechanism works so it operates in a
whileloop instead of making a recursive call, completely avoiding to run into
the growing stack issue.
Fixes aws/aws-cdk#8288
Fixes aws/aws-cdk#5187
Fixes aws/aws-cdk#8397
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.