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 3] Implement simulators as Durable Objects #656

Merged
merged 25 commits into from Sep 5, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Aug 14, 2023

Hey! πŸ‘‹ This PR ports all of Miniflare's simulators from Node.js to Durable Objects running in workerd. There should be practically no1 user-facing feature changes: this is purely an internal refactoring. This refactor brings a few advantages:

  • Removing native dependencies: Miniflare previously depended on better-sqlite3 for an SQLite implementation. workerd already ships with SQLite, and better-sqlite3 requires a native add-on. If prebuilt binaries were not found, this was compiled from source. On my 2021 M1 Pro MacBook, this takes just under 30s. Because miniflare is a hard-dependency of wrangler, users installing wrangler would often hit this, slowing down their installs. better-sqlite3 is a great library, but if we're already shipping SQLite with workerd, ideally we shouldn't need to ship it again. better-sqlite3 also ships with a fair few transitive dependencies. By removing our dependency on it, we're able to cut Miniflare's total dependency count in half:
    Miniflare 3's transitive dependencies
    (generated by https://npm.anvaka.com/#/view/2d/miniflare)

  • Running workerd standalone: we're now able to run Miniflare's simulators in workerd alone, without Node.js involvement. This means workerd users can benefit from Miniflare's simulators to run Workers written using Cloudflare's services. In a future PR, we'll add some sample capnp configs for doing this, allowing you to do things like using Miniflare = import "./node_modules/miniflare/miniflare.capnp"; to add simulators to your workerd config. These configs could be used in conjunction with workerd compile to build single-file-binaries for running Cloudflare Workers too.

  • Opening the door for production code sharing: lots of Cloudflare's Developer Platform is built on Cloudflare. By porting Miniflare's simulators to Workers, we're making it easier for us to use parts of the actual production implementations of KV, R2, Queues, and D1 in the future.

This PR is best reviewed commit-by-commit, I've added comments for particular areas of concern.

Footnotes

  1. Technically Queues is now supported on Node versions < 18, and the D1 database API has been updated to match the new, no-longer "experimental" backend ↩

@changeset-bot
Copy link

changeset-bot bot commented Aug 14, 2023

⚠️ No Changeset found

Latest commit: 4b4dc30

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@mrbbot mrbbot force-pushed the bcoll/tre-magic-proxy branch 2 times, most recently from 681ff91 to da9e434 Compare August 14, 2023 12:58
@mrbbot mrbbot force-pushed the bcoll/tre-durable-object-simulators branch from 120a0d0 to 6cf2b79 Compare August 14, 2023 13:52
packages/miniflare/src/index.ts Show resolved Hide resolved
packages/miniflare/src/index.ts Show resolved Hide resolved
packages/miniflare/src/index.ts Show resolved Hide resolved
packages/miniflare/src/plugins/cache/index.ts Show resolved Hide resolved
packages/miniflare/src/plugins/shared/index.ts Outdated Show resolved Hide resolved
packages/miniflare/src/workers/shared/index.worker.ts Outdated Show resolved Hide resolved
packages/miniflare/src/workers/shared/keyvalue.worker.ts Outdated Show resolved Hide resolved
@mrbbot mrbbot added the multiple reviews suggested PRs that should probably have at least 2 reviews label Aug 14, 2023
@mrbbot mrbbot requested a review from a team August 14, 2023 14:39
@mrbbot mrbbot marked this pull request as ready for review August 14, 2023 14:39
@threepointone
Copy link
Contributor

oh my god yes please, this will make our installs so much faster and reliable

@mrbbot mrbbot force-pushed the bcoll/tre-magic-proxy branch 2 times, most recently from 6498024 to cabbecb Compare August 16, 2023 14:11
@mrbbot mrbbot marked this pull request as draft August 16, 2023 14:15
@mrbbot mrbbot force-pushed the bcoll/tre-magic-proxy branch 2 times, most recently from 152e2de to 62c842e Compare August 17, 2023 11:24
Base automatically changed from bcoll/tre-magic-proxy to tre August 17, 2023 12:51
@mrbbot mrbbot force-pushed the bcoll/tre-durable-object-simulators branch 6 times, most recently from 6eb5436 to 9bd3a27 Compare August 17, 2023 16:52
@mrbbot mrbbot marked this pull request as ready for review August 17, 2023 16:57
mrbbot added a commit that referenced this pull request Aug 21, 2023
This is a temporary fix to allow Workers Sites tests cherrypicked
from #656 to pass. Listing keys in Workers Sites `__STATIC_CONTENT`
namespace was previously broken, due to a type error in the inline
namespace worker script. This change also means we can avoid the
`Function#toString()`ing to "bundle" code in inline worker scripts.
mrbbot added a commit that referenced this pull request Aug 21, 2023
mrbbot added a commit that referenced this pull request Aug 21, 2023
This is a temporary fix to allow Workers Sites tests cherrypicked
from #656 to pass. Listing keys in Workers Sites `__STATIC_CONTENT`
namespace was previously broken, due to a type error in the inline
namespace worker script. This change also means we can avoid the
`Function#toString()`ing to "bundle" code in inline worker scripts.
mrbbot added a commit that referenced this pull request Aug 21, 2023
mrbbot added a commit that referenced this pull request Aug 21, 2023
This is a temporary fix to allow Workers Sites tests cherrypicked
from #656 to pass. Listing keys in Workers Sites `__STATIC_CONTENT`
namespace was previously broken, due to a type error in the inline
namespace worker script. This change also means we can avoid the
`Function#toString()`ing to "bundle" code in inline worker scripts.
mrbbot added a commit that referenced this pull request Aug 21, 2023
Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working through this, but some initial comments

This replaces the loopback server's support for logging requests with
a more generic endpoint, that supports logging any arbitrary message.
This will be needed by Durable Object simulators that want to write
to the console, without `workerd`'s verbose prefixing. In particular,
Queues logs dispatches, and Cache logs first usage when the
`cacheWarnUsage` option is enabled.
Previously, we were only testing the `KVGateway`. In preparation for
porting the KV implementation to a worker, rewrite the tests to test
the full KV flow, including the router.
...and move dependencies only used in Workers to `devDependencies`.
They're bundled in with the Worker code, so don't need to be
installed again. Importantly, this change removes `better-sqlite3`,
meaning Miniflare no longer has any native dependencies other than
`workerd`.

Fixes #599

Fixes cloudflare/workers-sdk#3423
Fixes cloudflare/workers-sdk#3449
Fixes cloudflare/workers-sdk#3531
Fixes cloudflare/workers-sdk#3534
Fixes cloudflare/workers-sdk#3708
Fixes cloudflare/workers-sdk#3746

Closes cloudflare/cloudflare-docs#10241
When combined with running tests in serial, this should ensure we
only have at most two TCP connections open to `workerd` at a time.
@mrbbot mrbbot force-pushed the bcoll/tre-durable-object-simulators branch from ca5ba92 to 4b4dc30 Compare September 5, 2023 14:03
@mrbbot mrbbot merged commit 6ec2eaa into tre Sep 5, 2023
8 checks passed
@mrbbot mrbbot deleted the bcoll/tre-durable-object-simulators branch September 5, 2023 14:10
mrbbot added a commit that referenced this pull request Sep 6, 2023
With #656, the Queues dispatcher is now implemented as part of the
broker Durable Object. We no longer send message batches directly
from Node.js, so can remove queue handling from the entry worker.

Note the magic proxy enqueues messages through queue producer
bindings like regular workers, so never used this endpoint directly.
mrbbot added a commit that referenced this pull request Sep 11, 2023
With #656, the Queues dispatcher is now implemented as part of the
broker Durable Object. We no longer send message batches directly
from Node.js, so can remove queue handling from the entry worker.

Note the magic proxy enqueues messages through queue producer
bindings like regular workers, so never used this endpoint directly.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Nov 17, 2023
cloudflare/miniflare#656 introduced a bug in the SQL statement used
for getting the old blob ID of entries to delete when overriding keys.
This meant if a key was overridden, the old blob pointed to by that
key was not deleted. This lead to an accumulation of garbage when
`kvPersist` and `cachePersist` were enabled.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Dec 4, 2023
cloudflare/miniflare#656 introduced a bug in the SQL statement used
for getting the old blob ID of entries to delete when overriding keys.
This meant if a key was overridden, the old blob pointed to by that
key was not deleted. This lead to an accumulation of garbage when
`kvPersist` and `cachePersist` were enabled.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Dec 4, 2023
cloudflare/miniflare#656 introduced a bug in the SQL statement used
for getting the old blob ID of entries to delete when overriding keys.
This meant if a key was overridden, the old blob pointed to by that
key was not deleted. This lead to an accumulation of garbage when
`kvPersist` and `cachePersist` were enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multiple reviews suggested PRs that should probably have at least 2 reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants