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

UnhandledPromiseRejectionWarning: TypeError: reader._closedPromise_resolve is not a function #66

Closed
RangerMauve opened this issue Nov 11, 2020 · 5 comments · Fixed by #67
Assignees
Labels
bug domain: compliance Issues related to compliance with the streams standard
Milestone

Comments

@RangerMauve
Copy link

Hi, I'm getting an unhandled error even though it seems to be working otherwise.

``` (node:102333) UnhandledPromiseRejectionWarning: TypeError: reader._closedPromise_resolve is not a function at defaultReaderClosedPromiseResolve (/home/mauve/programming/make-fetch/node_modules/web-streams-polyfill/dist/ponyfill.es6.js:261:16) at ReadableStreamClose (/home/mauve/programming/make-fetch/node_modules/web-streams-polyfill/dist/ponyfill.es6.js:3092:9) at ReadableStreamDefaultControllerClose (/home/mauve/programming/make-fetch/node_modules/web-streams-polyfill/dist/ponyfill.es6.js:2616:13) at /home/mauve/programming/make-fetch/node_modules/web-streams-polyfill/dist/ponyfill.es6.js:3527:13 at processTicksAndRejections (internal/process/task_queues.js:97:5) (node:102333) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1) (node:102333) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. ```

This is happening with version 3.0.0 installed from NPM on Node v12.16.2

@MattiasBuelens
Copy link
Owner

That's odd... that shouldn't happen. Thanks for reporting.

Do you have a way to reproduce this problem? A code snippet that I can run, or a repository that I can clone?

@MattiasBuelens MattiasBuelens self-assigned this Nov 11, 2020
@RangerMauve
Copy link
Author

Thanks for the quick response.

It's happening in my tests in make-fetch

@MattiasBuelens
Copy link
Owner

I managed to reproduce it locally right away. Thanks for the very clear reproduction case! 😄

When the readable stream closes (through ReadableStreamClose), it calls the close steps of the pending read request of the async iterator, which releases the lock. This rejects the reader's closed promise, which also clears its resolve and reject callbacks. However, ReadableStreamClose then tries to resolve that same reader's closed promise, which fails because we already cleared those callbacks.

The polyfill assumes that we'll only ever try to resolve or reject the closed promise once. However, the specification doesn't seem to guarantee that, and your example shows that. So this assumption is incorrect, and the polyfill needs to handle this case. I'll work on a fix as soon as possible. 😉

On a slightly unrelated note: I also noticed that, although the closed promise was still pending, ReadableStreamReaderGenericRelease still created a new closed promise instead of rejecting the existing one. This is because ReadableStreamClose had already set the stream's state to "closed" before calling the close steps, which in turn call ReadableStreamReaderGenericRelease. I'll investigate the impact of this a bit further, since this might need a spec change. 🕵️‍♂️

@MattiasBuelens MattiasBuelens added bug domain: compliance Issues related to compliance with the streams standard labels Nov 11, 2020
@MattiasBuelens MattiasBuelens added this to the v3.0.1 milestone Nov 11, 2020
@RangerMauve
Copy link
Author

RangerMauve commented Nov 11, 2020

Jeeze, sounds pretty complex. Hopefully this'll help others in the future.

Is there anything I should do on my end of the code in the meantime? I'm fine with leaving my try-catch hack in there for now.

Thank you for investigating!

@MattiasBuelens
Copy link
Owner

It's quite complex, yes. I'm actually amazed something as complicated as this can be triggered with very little code. 😅

Your try-catch hack should be fine for now. Hopefully you'll be able to remove it after the next patch release. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug domain: compliance Issues related to compliance with the streams standard
Projects
None yet
2 participants