implement MessagePort and MessageChannel #3336

anonrig · 2025-01-14T19:33:16Z

Work in progress.

github-actions · 2025-01-14T19:36:46Z

The generated output of @cloudflare/workers-types has been changed by this PR. If this is intentional, run bazel build //types && rm -rf types/generated-snapshot && cp -r bazel-bin/types/definitions types/generated-snapshot to update the snapshot. Alternatively, you can download the full generated types:

Full Type Diff

src/workerd/api/BUILD.bazel

src/workerd/api/message-channel.h

src/workerd/api/message-channel.c++

src/workerd/api/message-channel.h

jasnell · 2025-01-14T21:15:28Z

No idea how all the comments got marked resolved. Very weird.

src/workerd/api/events.h

jasnell · 2025-01-14T21:36:31Z

src/workerd/api/message-channel.h

+  kj::Maybe<jsg::HashableV8Ref<v8::Object>> onmessageerror;
+
+  // Each MessagePort object can be entangled with another (a symmetric relationship)
+  kj::Maybe<jsg::Ref<MessagePort>> entangledWith{};


You'll need the visitForGc method implemented in this class also visiting each of these fields (onmessage, onmessageerror, and entagledWith)

jasnell · 2025-01-14T21:41:33Z

src/workerd/api/events.h

+ public:
+  CloseEvent(): Event("close") {}
+
+  CloseEvent(uint code, kj::String reason, bool clean)


It's worth adding code comments here indicating that these constructor variations are specific to websockets.

jasnell · 2025-01-14T21:42:07Z

src/workerd/api/message-channel.h

+  static jsg::Ref<MessagePort> constructor();
+
+  struct StructuredSerializeOptions {
+    kj::Array<jsg::Object> transfer{};


jsg::Object instances must always be held by a jsg::Ref... this should also be an optional argument also.

Suggested change

kj::Array<jsg::Object> transfer{};

jsg::Optional<kj::Array<jsg::Ref<jsg::Object>>> transfer{};

jasnell · 2025-01-14T21:44:51Z

src/workerd/api/message-channel.h

+
+  void postMessage(jsg::Lock& js,
+      jsg::Value message,
+      kj::OneOf<kj::Maybe<StructuredSerializeOptions>, kj::Array<jsg::Value>> options);


This should be inverted and made into a jsg::Optional ...

jsg::Optional<kj::OneOf<StructuredSerializeOptions, kj::Array<jsg::Value>> options

jasnell · 2025-01-14T21:46:22Z

src/workerd/api/message-channel.h

+  ~MessagePort() noexcept(false);
+  KJ_DISALLOW_COPY(MessagePort);
+
+  static jsg::Ref<MessagePort> constructor();


Per the spec, new MessagePort() should fail with an error (it is not user constructible). This should be removed.

Suggested change

static jsg::Ref<MessagePort> constructor();

jasnell · 2025-01-14T21:47:07Z

src/workerd/api/message-channel.c++

+MessageChannel::MessageChannel(jsg::Lock &js)
+    : port1(MessagePort::constructor()),
+      port2(MessagePort::constructor()) {}


Suggested change

MessageChannel::MessageChannel(jsg::Lock &js)

: port1(MessagePort::constructor()),

port2(MessagePort::constructor()) {}

MessageChannel::MessageChannel(jsg::Lock &js)

: port1(jsg::alloc<MessagePort>()),

port2(jsg::alloc<MessagePort>()) {}

These should be entangled with each other immediately after creation, shouldn't they?

jasnell · 2025-01-14T22:05:34Z

src/workerd/api/message-channel.c++

+jsg::Ref<MessagePort> MessagePort::constructor() {
+  return jsg::alloc<MessagePort>();
+}


MessagePort is not user-constructible so shouldn't implement this

Suggested change

jsg::Ref<MessagePort> MessagePort::constructor() {

return jsg::alloc<MessagePort>();

}

jasnell · 2025-01-14T22:06:36Z

src/workerd/api/message-channel.c++

+void MessagePort::disentangle(jsg::Lock &js) {
+  KJ_IF_SOME(e, entangledWith) {
+    // Fire an event named close at otherPort.
+    e->dispatchEvent(js, CloseEvent::constructor());


In general practice, the static constructor is meant to the be implementation of the new T constructor in javascript. For internal construction, use jsg::alloc<T> instead.

Suggested change

e->dispatchEvent(js, CloseEvent::constructor());

e->dispatchEvent(js, jsg::alloc<CloseEvent>());

jasnell · 2025-01-14T22:07:05Z

src/workerd/api/message-channel.c++

+  // If this is entangled, disentangle it.
+  disentangle(js);
+  // The close event will be fired even if the port is not explicitly closed.
+  dispatchEvent(js, CloseEvent::constructor());


Suggested change

dispatchEvent(js, CloseEvent::constructor());

dispatchEvent(js, jsg::alloc<CloseEvent>());

jasnell · 2025-01-14T22:08:57Z

src/workerd/api/message-channel.h

+  void close(jsg::Lock& js);
+
+  JSG_RESOURCE_TYPE(MessagePort) {
+    JSG_NESTED_TYPE(EventTarget);


Suggested change

JSG_NESTED_TYPE(EventTarget);

JSG_INHERIT(EventTarget)

jasnell · 2025-01-14T22:11:39Z

src/workerd/api/message-channel.h

+    JSG_READONLY_PROTOTYPE_PROPERTY(port1, getPort1);
+    JSG_READONLY_PROTOTYPE_PROPERTY(port2, getPort2);


Does the spec require these to be properties on the MessageChannel prototype? If not, using lazy instance properties would be more efficient. Not critical tho.

Suggested change

JSG_READONLY_PROTOTYPE_PROPERTY(port1, getPort1);

JSG_READONLY_PROTOTYPE_PROPERTY(port2, getPort2);

JSG_READONLY_LAZY_INSTANCE_PROPERTY(port1, getPort1);

JSG_READONLY_LAZY_INSTANCE_PROPERTY(port2, getPort2);

jasnell · 2025-01-14T22:13:02Z

src/workerd/api/message-channel.h

+// Ref: https://html.spec.whatwg.org/multipage/web-messaging.html#message-channels
+class MessageChannel: public jsg::Object {
+ public:
+  explicit MessageChannel(jsg::Lock& js);


the jsg::Lock& argument is not used.

anonrig requested a review from jasnell January 14, 2025 19:33