Skip to content

fix: do not auto destroy request stream - #132

Merged
devinivy merged 4 commits into
hapijs:masterfrom
achingbrain:fix/do-not-override-destroy-method
Sep 2, 2020
Merged

fix: do not auto destroy request stream#132
devinivy merged 4 commits into
hapijs:masterfrom
achingbrain:fix/do-not-override-destroy-method

Conversation

@achingbrain

@achingbrain achingbrain commented Aug 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

achingbrain commented Aug 26, 2020

Copy link
Copy Markdown
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.

@achingbrain achingbrain mentioned this pull request Aug 26, 2020
Closed
@achingbrain
fix: do not override destroy method
c8124cf 
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
@achingbrain achingbrain mentioned this pull request Aug 26, 2020
Merged
@kanongil

kanongil commented Aug 26, 2020

Copy link
Copy Markdown
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

achingbrain commented Aug 26, 2020

Copy link
Copy Markdown
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

kanongil commented Aug 26, 2020

Copy link
Copy Markdown
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

achingbrain commented Aug 26, 2020

Copy link
Copy Markdown
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

cjihrig commented Aug 29, 2020

Copy link
Copy Markdown
Contributor

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

@achingbrain

achingbrain commented Sep 1, 2020

Copy link
Copy Markdown
Contributor Author

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

@cjihrig

cjihrig commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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

achingbrain
achingbrain commented Sep 1, 2020
View reviewed changes
Comment thread lib/request.js Outdated
@devinivy @achingbrain
Revert changes in favor of hapijs#133, keeping new test
6d3d470 
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

devinivy commented Sep 2, 2020

Copy link
Copy Markdown
Member

@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

achingbrain commented Sep 3, 2020

Copy link
Copy Markdown
Contributor Author

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

@devinivy

devinivy commented Sep 3, 2020

Copy link
Copy Markdown
Member

@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

Reviewers

No reviews

Assignees

@devinivy devinivy

Labels

test Test or coverage

Projects

None yet

Milestone

5.0.4

Development

Successfully merging this pull request may close these issues.

Unparsed stream payload breaks under node 14

4 participants

@achingbrain @kanongil @cjihrig @devinivy