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.

Sign up for GitHub

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

Jump to bottom

Re-add reentrancy avoidance #23342

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Re-add reentrancy avoidance #23342

merged 2 commits into from
Feb 23, 2022

Commits on Feb 23, 2022

  1. tests: add failing test to demonstrate bug in ReadableStream implemen… 

    …tation
    @jplhomer @sebmarkbage
    jplhomer authored and sebmarkbage committed Feb 23, 2022
    Configuration menu
    Copy the full SHA
    5619560 View commit details
    Browse the repository at this point in the history

  2. Re-add reentrancy avoidance 

    I removed the equivalency of this in facebook#22446. However, I didn't fully
understand the intended semantics of the spec but I understand this better
now.

The spec is not actually recursive. It won't call pull again inside of a
pull. It might not call it inside startWork neither which the original
issue avoided. However, it will call pull if you enqueue to the controller
without filling up the desired size outside any call.

We could avoid that by returning a Promise from pull that we wait to
resolve until we've performed all our pending tasks. That would be the
more idiomatic solution. That's a bit more involved but since we know
understand it, we can readd the reentrancy hack since we have an easy place
to detect it. If anything, it should probably throw or log here otherwise.

I believe this fixes facebook#22772.

This includes the test from facebook#22889 but should ideally have one for Fizz.
    @sebmarkbage
    sebmarkbage committed Feb 23, 2022
    Configuration menu
    Copy the full SHA
    8ecc536 View commit details
    Browse the repository at this point in the history