-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: Set up listeners first, in case of long write to stdin. #520
Conversation
Closed as this does not seem to actually fix the bug. Investigating further. |
Re-opened: demo code now passes on this branch; fails on main. |
Hi @davesnx! do you mind reviewing this? The same issue might be affecting others trying to upgrade to Node 16. Thanks in advance! 🙏🏽 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, those cases are great to fix!
Let a few comments for having a better status of the code.
I don't think is a good idea to merge this without any test case. Let me know If you need help making it.
Thanks again!
src/exec.js
Outdated
process.stdin.on('error', err => { | ||
if (!promiseIsClosed) { | ||
promiseIsClosed = true | ||
return reject(err) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This onError should be only attached to process.stdin when there's stdin, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, yes. If there is not stdin, there will be no error.
src/exec.js
Outdated
// All of these handlers can close the Promise, so guard closing it twice. | ||
let promiseIsClosed = false | ||
|
||
process.on('error', err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about this. Reading your comments seems like the error happens when process is closed, not when it "errors". When this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this process.on('error'
is actually necessary; I can try removing it. What is necessary is the next handler, the process.stdin.on('error
.
The error happens when the process is closed, but is actually generated by the process's stdin. The chain of events is:
- We are still writing to stdin.
- Child process closes for some reason (such as an invalid jq filter).
- When the child process closes, its stdin closes as well.
- We continue to try to write to its stdin.
- Its stdin generates an error, because we're trying to write to it but it's been closed.
src/exec.js
Outdated
@@ -12,6 +12,33 @@ const exec = (command, args, stdin, cwd) => { | |||
|
|||
const process = childProcess.spawn(command, args, spawnOptions) | |||
|
|||
// All of these handlers can close the Promise, so guard closing it twice. | |||
let promiseIsClosed = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rename this to express better what it means?
isStdinClosed
or stdinClosedEarly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe promiseIsRejected
? I should've used better wording here; this is about making sure we don't call reject()
on the Promise twice.
Hi @davesnx, can you review again? I've refactored some things per your comments, and have added a unit test. If you comment out the |
I allowed the CI to run again (this isn't ideal :() If you need more help, DM me on Twitter https://twitter.com/davesnx |
const largeJsonString = JSON.parse(readFileSync(PATH_LARGE_JSON_FIXTURE)) | ||
run(FILTER_INVALID, largeJsonString, { input: 'json' }) | ||
.then(result => { | ||
done('Expected an error to be thrown from child process stdin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be here otherwise the test always pass green. Can you remove the done call, and maybe throw or handle this in another fashion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling done without any arguments passes a test, but calling it with arguments fails the test. Per the Mocha documentation, "This callback accepts both an Error instance (or subclass thereof) or a falsy value; anything else is invalid usage and throws an error (usually causing a failed test)."
I've tried this out locally - making the run(...)
Promise resolve so that this done
call is hit, and the test does fail in that case. It would be more proper to wrap this error message in an Error
, but either way this done call does fail the test.
🎉 This PR is included in version 2.3.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
We discovered a bug where large payloads can cause unhandled errors. This is a fix for that bug.
Root cause of bug
In the
exec
method defined insrc/exec.js
, the child process is spawned, then the JSON data is sent to stdin, and then listeners are added. The problem here is that the child process can exit while we are still writing to its stdin. If this happens, its stdin will also close, and will generate a low-levelEPIPE
error event, as we will be writing to a closed pipe. Nothing will handle that error event.Steps to reproduce
The following code should reproduce the bug:
Expected result:
With a big enough payload, we should get and catch the EPIPE:
Actual result:
But without the bugfix, you get this:
Notes
With a smaller payload, the bug is not triggered, and we see this:
Node versioning note
On older versions of Node, this sort of unhandled error event may have been ignored. On Node v16, it will crash the application.