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: do not auto destroy request stream #132

Merged

Conversation

achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Aug 26, 2020

Auto destroying the request stream stops for await..of-style iteration over request payloads working when output: 'stream', parse: false are used as request handler payload config options.

Fixes hapijs/hapi#4149

@achingbrain
Copy link
Contributor Author

This is another attempt to resolve the node 14 streaming bug - it's smaller scope than #129 and only addresses the issue I encountered.

The hapi test suite from master passes with this change made to shot.

The node docs say:

> Implementors should not override this method, but instead implement readable._destroy()

Fixes hapijs/hapi#4149
@achingbrain achingbrain force-pushed the fix/do-not-override-destroy-method branch from 958ab92 to c8124cf Compare August 26, 2020 07:22
@kanongil
Copy link
Contributor

This is not the right fix. The simple fix, that preserves as much as possible, is to just add { autoDestroy: false} to the super() call. This effectively reverts this node 14.0 feature for shot.

@achingbrain
Copy link
Contributor Author

just add { autoDestroy: false } to the super() call.

I've added this to the constructor and removed the destroy method since the node docs say it shouldn't be overridden.

autoDestroy was added in node 10 so it should work for supported versions.

@kanongil
Copy link
Contributor

Please don't remove the destroy overload for a minimal patch, as it could create extra 'close' emits (from autoClose: true), and other unforeseen changes.

@achingbrain
Copy link
Contributor Author

I've restored it, though the docs say not to override it.

@achingbrain achingbrain changed the title fix: do not override destroy method fix: do not auto close request stream Aug 26, 2020
@achingbrain achingbrain changed the title fix: do not auto close request stream fix: do not auto destroy request stream Aug 26, 2020
@cjihrig
Copy link
Contributor

cjihrig commented Aug 29, 2020

I think we should go with something more like #133, but I'd like to keep the test here.

@achingbrain
Copy link
Contributor Author

I have no strong preference either way, I'd just like the bug to be squashed!

@cjihrig
Copy link
Contributor

cjihrig commented Sep 1, 2020

#133 has a handful of reviews. Can you please revert the changes to lib/ in this PR so we can keep the test.

lib/request.js Outdated Show resolved Hide resolved
Co-authored-by: Alex Potsides <alex@achingbrain.net>
@devinivy devinivy added this to the 5.0.4 milestone Sep 2, 2020
@devinivy devinivy added the test Test or coverage label Sep 2, 2020
@devinivy
Copy link
Member

devinivy commented Sep 2, 2020

@achingbrain thanks for the contribution and for bearing with all the back-and-forth on this one. This bug turned out to be a tricky one.

@devinivy devinivy self-assigned this Sep 2, 2020
@devinivy devinivy merged commit d7080c8 into hapijs:master Sep 2, 2020
@achingbrain achingbrain deleted the fix/do-not-override-destroy-method branch September 2, 2020 12:28
@achingbrain
Copy link
Contributor Author

We got there in the end, thanks for all the help. Will this be released soon?

@devinivy
Copy link
Member

devinivy commented Sep 3, 2020

@achingbrain Yeah, I expect it will. Someone just has to go through the steps of releasing shot and a patch version of hapi. My guess is that it will be settled before Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Test or coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unparsed stream payload breaks under node 14
4 participants