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

[miniflare] fix: ensure Mutex doesn't report itself as drained if locked #4321

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Nov 1, 2023

What this PR solves / how to test:

Previously, Miniflare's Mutex implementation would report itself as drained if there were no waiters, regardless of the locked state. This bug meant that if you called but didn't await Miniflare#setOptions(), future calls to Miniflare#dispatchFetch() (or any other asynchronous Miniflare method) wouldn't wait for the options update to apply and the runtime to restart before sending requests. This change ensures we wait until the mutex is unlocked before reporting it as drained.

Author has addressed the following:

  • Tests
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • Included
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because: this is a bug fix

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@mrbbot mrbbot requested a review from a team as a code owner November 1, 2023 17:05
Copy link

changeset-bot bot commented Nov 1, 2023

🦋 Changeset detected

Latest commit: 9ce910c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
miniflare Patch
@cloudflare/pages-shared Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 1, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6731655979/npm-package-wrangler-4321

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6731655979/npm-package-wrangler-4321

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6731655979/npm-package-wrangler-4321 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6731655979/npm-package-miniflare-4321
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6731655979/npm-package-cloudflare-pages-shared-4321

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.15.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20231025.1
workerd 1.20231025.0 1.20231025.0
workerd --version 1.20231025.0 2023-10-25

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

Previously, magic proxies were only poisoned once the runtime had
restarted. This meant it was possible for heap objects to be
freed while they were still in use, e.g.

1. `const caches = await Miniflare#getCaches()` _(create proxy)_
2. `await caches.default.put(...)` _(resolve `put()` function)_
3. `void Miniflare#setOptions()` _(in background)_
4. Either...
   a. `caches` proxy GCed, `FinalizationRegistry` callback called,
      not yet poisoned, so calls `dispatchFetch()`, but
      `dispatchFetch()` blocked until `setOptions()` completes
   b. `caches.default.put(...)`, calls `dispatchFetch()`, but
      `dispatchFetch()` blocked until `setOptions()` completes
5. `setOptions()` completes, poisoning proxies, but too late
6. `dispatchFetch()` unblocked, sends request to incorrect DO

The "blocked until `setOptions()` completes" step was fixed by the
previous commit, hence we're only seeing this bug now.

This change ensures we poison proxies synchronously as soon as
`setOptions()` is called, ensuring the problematic `dispatchFetch()`
calls never happen.
@mrbbot
Copy link
Contributor Author

mrbbot commented Nov 2, 2023

This test failures from this fix exposed another race condition. 😃 See the description of 9ce910c for details. Another potential fix I considered to eliminate this class of "request-sent-to-incorrect-Durable-Object-bug" was to add an id = crypto.randomUUID() property to the ProxyServer object, send this back when return native targets from the proxy, and require it to be sent when performing actions on the proxy. If the ID didn't match the current object, an error would be thrown.

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #4321 (9ce910c) into main (a4a89ae) will increase coverage by 0.04%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4321      +/-   ##
==========================================
+ Coverage   75.34%   75.38%   +0.04%     
==========================================
  Files         223      223              
  Lines       12341    12341              
  Branches     3190     3190              
==========================================
+ Hits         9298     9303       +5     
+ Misses       3043     3038       -5     

see 4 files with indirect coverage changes

@mrbbot mrbbot requested a review from RamIdeas November 2, 2023 11:41
@mrbbot mrbbot merged commit 29a59d4 into main Nov 2, 2023
19 of 20 checks passed
@mrbbot mrbbot deleted the bcoll/miniflare-mutex-drain-no-waiter-fix branch November 2, 2023 12:26
@workers-devprod workers-devprod mentioned this pull request Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants