Move implementation of queue event handler into workerd #543

a-robinson · 2023-04-14T21:58:36Z

This required a few more changes than the binding did, so merits at least a slightly closer review. All internal tests pass, at least.

src/workerd/api/queue.h

src/workerd/api/queue.c++

a-robinson · 2023-04-16T04:25:38Z

Thanks for taking a look, @jasnell !

mrbbot

Nice! Thanks again for picking this up! 😃 Left some comments about types that can now be auto-generated. 👍

mrbbot · 2023-04-17T09:39:38Z

src/workerd/api/queue.h

+    JSG_READONLY_INSTANCE_PROPERTY(body, getBody);
+    JSG_METHOD(retry);
+    JSG_METHOD(ack);
+  }


Suggested change

}

JSG_TS_OVERRIDE(Message<Body = unknown> {

readonly body: Body;

});

}

...and remove:

workerd/types/defines/queues.d.ts

Lines 1 to 25 in 2a71783

/**

* A message that is sent to a consumer Worker.

*/

interface Message<Body = unknown> {

/**

* A unique, system-generated ID for the message.

*/

readonly id: string;

/**

* A timestamp when the message was sent.

*/

readonly timestamp: Date;

/**

* The body of the message.

*/

readonly body: Body;

/**

* Marks message to be retried.

*/

retry(): void;

/**

* Marks message acknowledged.

*/

ack(): void;

}

mrbbot · 2023-04-17T09:42:41Z

src/workerd/api/queue.h

+
+    JSG_METHOD(retryAll);
+    JSG_METHOD(ackAll);
+  }


Suggested change

}

JSG_TS_OVERRIDE(MessageBatch<Body = unkown> {

readonly messages: readonly Message<Body>[];

});

}

...and remove:

workerd/types/defines/queues.d.ts

Lines 27 to 47 in 2a71783

/**

* A batch of messages that are sent to a consumer Worker.

*/

interface MessageBatch<Body = unknown> {

/**

* The name of the Queue that belongs to this batch.

*/

readonly queue: string;

/**

* An array of messages in the batch. Ordering of messages is not guaranteed.

*/

readonly messages: readonly Message<Body>[];

/**

* Marks every message to be retried in the next batch.

*/

retryAll(): void;

/**

* Marks every message acknowledged in the batch.

*/

ackAll(): void;

}

This also needed a JSG_TS_ROOT() and for this file to be included in global-scope.h, but after that it looks good 👍

src/workerd/api/queue.h

mrbbot · 2023-04-17T09:52:47Z

src/workerd/api/queue.h

+
+    JSG_METHOD(retryAll);
+    JSG_METHOD(ackAll);
+  }


Suggested change

}

JSG_TS_OVERRIDE(<Body = unknown> {

readonly messages: readonly Message<Body>[];

});

}

Is it intentional that you're not specifying a name here?

It looks like before this code move, we were just defining one type (MessageBatch that effectively both QueueEvent and QueueController implemented. Is there a need to export this type at all given how we're renaming QueueController's type below?

FWIW this is currently being ignored due to the lack of a JSG_TS_ROOT(), and that seems ok?

Given this inherits ExtendableEvent, I suspect we do want a separate type, like we do with ScheduledEvent/ScheduledController. The renaming of QueueController was to match the existing type and avoid a breaking change. I also think we'll need to add an entry for QueueEvent here:

workerd/src/workerd/api/global-scope.h

Lines 92 to 97 in a363226

JSG_TS_DEFINE(type WorkerGlobalScopeEventMap = {

fetch: FetchEvent;

scheduled: ScheduledEvent;

unhandledrejection: PromiseRejectionEvent;

rejectionhandled: PromiseRejectionEvent;

});

Cool, done.

a-robinson

Thanks for the help with types!

src/workerd/api/queue.h

a-robinson · 2023-04-17T17:37:37Z

src/workerd/api/queue.h

+
+    JSG_METHOD(retryAll);
+    JSG_METHOD(ackAll);
+  }


Is it intentional that you're not specifying a name here?

It looks like before this code move, we were just defining one type (MessageBatch that effectively both QueueEvent and QueueController implemented. Is there a need to export this type at all given how we're renaming QueueController's type below?

FWIW this is currently being ignored due to the lack of a JSG_TS_ROOT(), and that seems ok?

a-robinson · 2023-04-17T17:56:01Z

src/workerd/api/queue.h

+    JSG_READONLY_INSTANCE_PROPERTY(body, getBody);
+    JSG_METHOD(retry);
+    JSG_METHOD(ack);
+  }


a-robinson · 2023-04-17T17:56:27Z

src/workerd/api/queue.h

+
+    JSG_METHOD(retryAll);
+    JSG_METHOD(ackAll);
+  }


This also needed a JSG_TS_ROOT() and for this file to be included in global-scope.h, but after that it looks good 👍

This required a few more changes than the binding did, so merits at least a slightly closer review. All internal tests pass, at least.

a-robinson · 2023-04-18T18:50:24Z

@mrbbot or @jasnell is either of you up for approving this? Josh's doesn't count since he isn't a maintainer.

a-robinson requested review from mrbbot and jbwcloudflare April 14, 2023 21:58

jasnell reviewed Apr 15, 2023

View reviewed changes

src/workerd/api/queue.h Outdated Show resolved Hide resolved

jasnell reviewed Apr 15, 2023

View reviewed changes

src/workerd/api/queue.h Outdated Show resolved Hide resolved

jasnell reviewed Apr 15, 2023

View reviewed changes

src/workerd/api/queue.h Outdated Show resolved Hide resolved

jasnell reviewed Apr 15, 2023

View reviewed changes

src/workerd/api/queue.c++ Show resolved Hide resolved

a-robinson force-pushed the arobinson/queue-handler branch from 559cf98 to d55213b Compare April 16, 2023 04:25

mrbbot reviewed Apr 17, 2023

View reviewed changes

a-robinson force-pushed the arobinson/queue-handler branch from cf2173b to 71eea19 Compare April 17, 2023 17:57

a-robinson commented Apr 17, 2023

View reviewed changes

a-robinson mentioned this pull request Apr 17, 2023

Support calling the queue() handler of a worker via a service binding #545

Merged

jbwcloudflare approved these changes Apr 18, 2023

View reviewed changes

Move implementation of queue event handler into workerd

87a27a9

This required a few more changes than the binding did, so merits at least a slightly closer review. All internal tests pass, at least.

a-robinson force-pushed the arobinson/queue-handler branch from 1d14f58 to 87a27a9 Compare April 18, 2023 16:56

mrbbot approved these changes Apr 19, 2023

View reviewed changes

a-robinson merged commit 7c8fe9d into main Apr 19, 2023
6 checks passed

mrbbot mentioned this pull request May 4, 2023

[Miniflare 3] Implement Queues cloudflare/miniflare#566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move implementation of queue event handler into workerd #543

Move implementation of queue event handler into workerd #543

a-robinson commented Apr 14, 2023

a-robinson commented Apr 16, 2023

mrbbot left a comment •

edited

Loading

mrbbot Apr 17, 2023 •

edited

Loading

a-robinson Apr 17, 2023

mrbbot Apr 17, 2023

a-robinson Apr 17, 2023

mrbbot Apr 17, 2023 •

edited

Loading

a-robinson Apr 17, 2023

mrbbot Apr 17, 2023

a-robinson Apr 18, 2023

a-robinson left a comment

a-robinson Apr 17, 2023

a-robinson Apr 17, 2023

a-robinson Apr 17, 2023

a-robinson commented Apr 18, 2023

-  }
+    JSG_TS_OVERRIDE(Message<Body = unknown> {
+      readonly body: Body;
+    });
+  }

	/**
	* A message that is sent to a consumer Worker.
	*/
	interface Message<Body = unknown> {
	/**
	* A unique, system-generated ID for the message.
	*/
	readonly id: string;
	/**
	* A timestamp when the message was sent.
	*/
	readonly timestamp: Date;
	/**
	* The body of the message.
	*/
	readonly body: Body;
	/**
	* Marks message to be retried.
	*/
	retry(): void;
	/**
	* Marks message acknowledged.
	*/
	ack(): void;
	}

-  }
+    JSG_TS_OVERRIDE(MessageBatch<Body = unkown> {
+      readonly messages: readonly Message<Body>[];
+    });
+  }

	/**
	* A batch of messages that are sent to a consumer Worker.
	*/
	interface MessageBatch<Body = unknown> {
	/**
	* The name of the Queue that belongs to this batch.
	*/
	readonly queue: string;
	/**
	* An array of messages in the batch. Ordering of messages is not guaranteed.
	*/
	readonly messages: readonly Message<Body>[];
	/**
	* Marks every message to be retried in the next batch.
	*/
	retryAll(): void;
	/**
	* Marks every message acknowledged in the batch.
	*/
	ackAll(): void;
	}

	JSG_TS_DEFINE(type WorkerGlobalScopeEventMap = {
	fetch: FetchEvent;
	scheduled: ScheduledEvent;
	unhandledrejection: PromiseRejectionEvent;
	rejectionhandled: PromiseRejectionEvent;
	});

Move implementation of queue event handler into workerd #543

Move implementation of queue event handler into workerd #543

Conversation

a-robinson commented Apr 14, 2023

a-robinson commented Apr 16, 2023

mrbbot left a comment • edited Loading

Choose a reason for hiding this comment

mrbbot Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbbot Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-robinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-robinson commented Apr 18, 2023

mrbbot left a comment •

edited

Loading

mrbbot Apr 17, 2023 •

edited

Loading

mrbbot Apr 17, 2023 •

edited

Loading