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

Fix stream error propagation for node v16+ #96

Merged
merged 3 commits into from Jun 20, 2022

Conversation

devinivy
Copy link
Member

@devinivy devinivy commented May 30, 2022

There was a failing test for node v16+. The issue is that starting in node v16, when a request is aborted it will not only emit an aborted event, but also an error event on req. That error event would propagate to other streams due to subtext's stream error propagation, and eventually surface as a separate error. The fix here is to remove the stream error propagation when handling the abort, by not only unpiping the streams, but also removing the error handlers that were setup while piping.

@devinivy devinivy added the bug Bug or defect label May 30, 2022
@devinivy devinivy added this to the 7.0.4 milestone May 30, 2022
@kanongil
Copy link
Contributor

Oh watch out – there be dragons here!

The underlying change is a major reason why I haven't dared upgrade to v16 in production. Unfortunately I don't have the energy to look further into it, but you can see some of my thoughts in nodejs/node#40775 (comment).

@devinivy
Copy link
Member Author

I appreciate you calling that out! Yeah, it's a little tricky ensuring http streams work on node v14 and node v16+. I was referencing those abort-related issues in hapi and nodejs, and specific changes to the node http code while coming-up with this fix.

Luckily in subtext I believe we're really just concerned with reading the request before it's aborted, and we don't care as much about handling an abort after the payload is read but before the server has responded, although that is a case that hapi is interested in. (We don't even see res in subtext, and I don't think that we need to.)

I see this change as being a general improvement— ensuring that piping then unpiping has no side-effects— and it happens to address the case that both an 'aborted' and 'error' event are emitted (node v16+) rather than just an 'aborted' event (node v14). The way this fix works is that 'aborted' causes unpiping and file cleanup, which means that an error does not need to be propagated from res to the write stream for the file. Previously an 'error' event with Error: aborted was being propagated to the file write stream, a side-effect of piping then unpiping that I believe should not have been happening anyway.

Does that track for you too?


from.removeListener('error', forwardError);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line here is really what this fix is all about. Previously we would unpipe but continue to forward along errors to other streams, and now we stop that error propagation when unpiping.

@devinivy devinivy merged commit 72b50bb into master Jun 20, 2022
@devinivy devinivy deleted the fix-stream-error-node-16 branch June 20, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants