Cleanup event handlers before return. #8
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
walac
force-pushed
the
cleanup-listeners
branch
2 times, most recently
from
7e1982c to
f321c57
Compare
|
Tests are failing but it feels like it is because one of the tests run against node 4, which seems not supported anymore. |
added 2 commits
November 2, 2017 14:22
Node 4 is old and it seems the project doesn't care about it anymore.
Update transform is designed to fail when a specific string is passed. It tests for that case and if the string matches, it calls the callback with an appropriate error object but, instead of returning, it keeps goin and calls the callback again, raising a 'write callback called multiple times' error. Add a return statement after calling the callback with error object to avoid disruptive behavior.
walac
force-pushed
the
cleanup-listeners
branch
from
f321c57 to
c2d7585
Compare
|
Pushed a commit to remove node 4. |
walac
force-pushed
the
cleanup-listeners
branch
2 times, most recently
from
90c1395 to
a147822
Compare
After the promisePipe returns, it leaves some event handlers installed in the streams. If the user later wants to reuse the streams, the handlers will be called out of proper contexts, which can lead to bad code behavior for no clear sane reason. We fix this by uninstalling all event handlers before the function returns or when an error happens. Notice that any prior event handler the user installed for the relevant events will be lost after the function returns. Fixing this would require a somewhat bigger code change, which seems not worth to cover such corner case.
walac
force-pushed
the
cleanup-listeners
branch
from
a147822 to
a410248
Compare
|
A more careful reading of the stream API showed that contrary to common sense in other stream APIs, a stream in nodejs cannot be reused once it is piped. So, if this is right, this PR has no practical effect. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After the promisePipe returns, it leaves some events handlers installed
in the streams. If the user later wants to reuse the streams, the
handlers will be called out of proper contexts, which can lead to bad
code behavior for no clear sane reason.
We fix this by uninstalling all event handlers before the function
returns or when an error happens.
Notice that any prior event handler the user installed for the relevant
events will be lost after the function returns. Fixing this would
require a somewhat bigger code change, which seems not worth
to cover such corner case.