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 Queues #566

Merged
merged 2 commits into from
May 12, 2023
Merged

[Miniflare 3] Implement Queues #566

merged 2 commits into from
May 12, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented May 4, 2023

This PR adds support for Queues to Miniflare 3. It builds on cloudflare/workerd#541, cloudflare/workerd#543, and cloudflare/workerd#545 (thanks @a-robinson!).

cloudflare/workerd#545 adds a queue() method on service bindings to dispatch queue events to Workers. It expects regular, de-serialised JavaScript objects, but the Queue producer bindings from cloudflare/workerd#541 send V8-serialised objects instead. Unfortunately, workerd doesn't expose the V8 (de)serialiser in Workers yet, so we need to re-serialise all messages into a format we can deserialise in a Worker. We use devalue for this. This is the first time we're using an npm dependency in an internal Miniflare Worker. To support this, and because it's something we've been wanting to do for a while, the core entrypoint Worker has been moved to a separate TypeScript file, which is built as an additional entrypoint with esbuild, and type-checked under @cloudflare/workers-types. This is a separate commit to aid reviewing. We'll migrate other internal Workers to this format soon.

Another minor issue is that workerd uses V8 serialisation version 15 when sending messages. This is only supported by V8 versions 10.0.29 and above. For reference, the V8 versions associated with notable Node versions are:

  • Miniflare's minimum supported version: Node 16.13.0 --> V8 9.4
  • Last Node 17/unsupported version: Node 17.9.1 --> V8 9.6
  • First supported version: Node 18.0.0 --> V8 10.1

This means that Queues will only be supported in Miniflare 3 if you're using Node 18 or above. I think this is ok, as I'm pretty sure Queues is still in beta, and it was previously marked experimental in Miniflare 2.


I've added some other questions that would be good to get opinions on too. 🙂

@mrbbot mrbbot added the tre Relating to Miniflare 3 label May 4, 2023
@changeset-bot
Copy link

changeset-bot bot commented May 4, 2023

⚠️ No Changeset found

Latest commit: 7fa2b72

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

Base automatically changed from tre-new-storage-r2 to tre May 9, 2023 14:20
@mrbbot mrbbot force-pushed the tre-queues branch 3 times, most recently from d23c667 to b6ce395 Compare May 9, 2023 15:42
for (const [queueName, consumer] of queueConsumers) {
if (consumer.deadLetterQueue !== undefined) {
// Check the dead letter queue isn't configured to be the queue itself
// (NOTE: Queues *does* permit DLQ cycles between multiple queues,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Queues does permit dead-letter queue cycles between multiple queues, but is this something we want to block in Miniflare? Are developers ever likely to need this?

else if (acked > 0) colour = yellow;
else colour = red;

let message = `${bold("QUEUE")} ${queueName} ${colour(`${acked}/${total}`)}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this log include anything else?

Comment on lines +235 to +229
enqueueOn: QueueEnqueueOn,
consumer: QueueConsumer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we wouldn't need to pass enqueueOn and consumer each time. The problem is that gateways are created once and then reused across the lifetime of a Miniflare instance. If the consumer ever changes from for example a setOptions() call, we need to respect that. I wonder if there's a better way of doing this... 🤔

Comment on lines +16 to +24
queueProducers: z
.union([z.record(z.string()), z.string().array()])
.optional(),
queueConsumers: z
.union([z.record(QueueConsumerOptionsSchema), z.string().array()])
.optional(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a slightly different API to Miniflare 2...

export interface BindingOptions {
name: string;
queueName: string;
}
export interface ConsumerOptions {
queueName: string;
maxBatchSize?: number;
maxWaitMs?: number;
maxRetries?: number;
deadLetterQueue?: string;
}
export interface QueuesOptions {
queueBindings?: BindingOptions[];
queueConsumers?: (string | ConsumerOptions)[];
}

...but I think it more closely matches the rest of Miniflare's options. Queues was previously marked as experimental, so I think this change is ok, but open to suggestions.

export const _QUEUES_COMPATIBLE_V8_VERSION =
semiver(process.versions.v8, "10.0.29") >= 0;

function assertCompatibleV8Version() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we'd just bump Miniflare 3's minimum supported Node version to 18, but I'm not sure if we want to do that for Wrangler 3 too?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's required for Queues...

packages/tre/src/workers/core/devalue.ts Show resolved Hide resolved
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.

Looking good! How does this work with Wrangler? i.e. if we don't have both workers available as source

packages/tre/src/plugins/core/errors/index.ts Show resolved Hide resolved
packages/tre/src/plugins/core/index.ts Outdated Show resolved Hide resolved
packages/tre/src/plugins/core/index.ts Show resolved Hide resolved
packages/tre/src/workers/core/constants.ts Show resolved Hide resolved
packages/tre/src/plugins/shared/constants.ts Outdated Show resolved Hide resolved
packages/tre/src/workers/core/devalue.ts Show resolved Hide resolved
Comment on lines +1 to +5
// @ts-expect-error "devalue" is ESM-only, so TypeScript requires us to use the
// `*.mts` extension, or set `"type": "module"` in our `package.json`. We can't
// do the first as ambient types from `@cloudflare/workers-types` don't seem to
// work, and the second would affect all consumers of `miniflare`.
import { unflatten } from "devalue";
Copy link
Contributor

Choose a reason for hiding this comment

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

https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#moduleresolution-bundler should fix this if the Typescript version can be upgraded to 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice! Doesn't look like api-extractor supports TypeScript 5 yet, but we should definitely switch to this when we upgrade. Alternatively, we just scrap API extractor and ship multiple *.d.ts files for our single bundled *.js file?

Copy link
Contributor

Choose a reason for hiding this comment

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

What would that look like? What's the benefit of api extractor over that approach?

Copy link
Contributor Author

@mrbbot mrbbot May 12, 2023

Choose a reason for hiding this comment

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

api-extractor just means we have a single index.d.ts file corresponding to our index.js bundle. Without it, the structure of our src folder would make it to the final bundle as TypeScript definitions file. Essentially, I think you'd be able to do stuff like import { ... } from "@miniflare/tre/plugins/core" as plugins/core/index.d.ts would exist, but the code for it would be in the index.js bundle, so it wouldn't actually work.

@mrbbot mrbbot requested a review from penalosa May 12, 2023 01:11
@mrbbot
Copy link
Contributor Author

mrbbot commented May 12, 2023

How does this work with Wrangler? i.e. if we don't have both workers available as source

Are you referring to the bundling of Miniflare into Wrangler here? I think the esbuild plugin for building Workers should alleviate this issue.

@penalosa
Copy link
Contributor

No, I was wondering how this works if Wrangler only has access to one user Worker—do the queue producer and consumer have to be defined in the same script?

@mrbbot
Copy link
Contributor Author

mrbbot commented May 12, 2023

Ah, yeah they would have to be. We could probably do something similar to remote Durable Objects by defining a fake queue handler that proxies to another process which then sends the messages to the correct handler.

@mrbbot
Copy link
Contributor Author

mrbbot commented May 12, 2023

Squashing...

@mrbbot
Copy link
Contributor Author

mrbbot commented May 12, 2023

Hmmm, that Windows test failure on Node 20 looks like a real issue... 🙁 Time to debug...

@mrbbot
Copy link
Contributor Author

mrbbot commented May 12, 2023

Potential fix here: cloudflare/workerd#644

@mrbbot
Copy link
Contributor Author

mrbbot commented May 12, 2023

I think I'm going to merge this with broken tests... The fix for this will require a workerd release, and upgrading Miniflare to support some of the other changes in that (e.g. R2 multiple etag support) requires separate changes. This will also unblock reviewing the other PRs that depend on this change. 👍

@mrbbot mrbbot merged commit 5268fc0 into tre May 12, 2023
7 of 8 checks passed
@mrbbot mrbbot deleted the tre-queues branch May 12, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tre Relating to Miniflare 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants