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

Worker MessageChannel #6691

Closed
rracariu opened this issue Jul 9, 2020 · 28 comments · Fixed by #11051
Closed

Worker MessageChannel #6691

rracariu opened this issue Jul 9, 2020 · 28 comments · Fixed by #11051
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)

Comments

@rracariu
Copy link

rracariu commented Jul 9, 2020

Seems that MessageChannel in missing from Deno APIs.

https://developer.mozilla.org/en-US/docs/Web/API/MessageChannel

It should be added as it allows safe communication between Workers

@lucacasonato
Copy link
Member

Structured cloning support is a prerequisite for MessageChannel AFAIK. Ref #3557

@ry ry added cli related to cli/ dir feat new feature (which has been agreed to/accepted) labels Jul 9, 2020
KallynGowdy added a commit to KallynGowdy/casualos that referenced this issue Jul 15, 2020
- Doesn't work because Deno doesn't support MessageChannel yet (denoland/deno#6691) - which is something Comlink wants.
@ghost

This comment has been minimized.

@mhmdmar
Copy link

mhmdmar commented Mar 7, 2021

@lucacasonato Is there any plan to add this API soon ?

@ghost
Copy link

ghost commented Mar 7, 2021

@mhmdmar The prerequisite (#3557) hasn't been finished yet, so not much can be done until then.

@mhmdmar
Copy link

mhmdmar commented Mar 7, 2021

@mhmdmar The prerequisite (#3557) hasn't been finished yet, so not much can be done until then.

Thank you for your response :)

@surma
Copy link
Contributor

surma commented Jun 18, 2021

With #3557 being closed now, is there anything else blocking MessageChannel from being implementable? I’m interested in helping out here (if desired), as I am keen to get Comlink working with Deno.

@lucacasonato
Copy link
Member

@surma I was just looking into it today. The main issue is that we do not support transferable for Worker#postMessage yet, so MessageChannel will not be super useful as the MessagePort can't be transferred between workers. What do you need to transfer between workers? Just the port, or also SAB?

@surma
Copy link
Contributor

surma commented Jun 18, 2021

Oh of course, you are right. Transferables!

For Comlink itself I only need MessagePort. In user-space, the user can transfer any transferable using Comlink.transfer(). But I reckon the vast majority of Comlink use-cases don’t use transferables. So just having MessagePort support would allow me to get Comlink up and running.

@bartlomieju
Copy link
Member

The main issue is that we do not support transferable for Worker#postMessage yet, so MessageChannel will not be super useful as the MessagePort can't be transferred between workers.

That said I don't think there are any more blockers for that functionality. I remember that @inteon was looking into that at some point.

@lucacasonato
Copy link
Member

I am sitting in a train with nothing better to do, so I'll give it a shot 🙃

@lucacasonato
Copy link
Member

Got distracted and implemented SharedArrayBuffer sharing between Workers instead: #11040. Might be useful for WASM threads for someone. Have a pretty clear idea on how to do MessageChannel now though, so that will be up next.

@inteon
Copy link
Contributor

inteon commented Jun 19, 2021

Transferring MessageChannel via postMessage() probably can be done by implementing write_host_object and read_host_object. Although I'm not 100% sure this will just work.
And ofc all other logic (not related to transfer) also still needs to be added.
#10750 is related -> same issue for BroadcastChannel instead of MessageChannel

@lucacasonato
Copy link
Member

@inteon Transferring MessageChannel happens via the transferable second argument to postMessage, not via the first argument that is structured cloned. read_host_object and write_host_object will only be needed for handling items that have the [Serializer] WebIDL extended attribute. MessageChannel doesn't have this, it only has [Transferable].

I will open a WIP PR that adds MessageChannel and MessagePort very soon - just cleaning some stuff up right now. But to make them work properly with workers I will need to rewrite parts of our web worker implementation again - so might be couple more days until its actually ready.

@inteon
Copy link
Contributor

inteon commented Jun 19, 2021

@inteon Transferring MessageChannel happens via the transferable second argument to postMessage, not via the first argument that is structured cloned. read_host_object and write_host_object will only be needed for handling items that have the [Serializer] WebIDL extended attribute. MessageChannel doesn't have this, it only has [Transferable].

I will open a WIP PR that adds MessageChannel and MessagePort very soon - just cleaning some stuff up right now. But to make them work properly with workers I will need to rewrite parts of our web worker implementation again - so might be couple more days until its actually ready.

Thank you for explaining.
So structured cloning is only important for encoding the messages over the MessageChannel not for transferring the ports of the MessageChannel, that is done via the second argument to postMessage.

@ghost
Copy link

ghost commented Jun 20, 2021

Got distracted and implemented SharedArrayBuffer sharing between Workers instead: #11040. Might be useful for WASM threads for someone. Have a pretty clear idea on how to do MessageChannel now though, so that will be up next.

Actually, since you'd brought up WebAssembly threading, Wasm doesn't directly use JavaScript (Shared)ArrayBuffer objects. JavaScript WebAssembly.Memory objects are supposed to be passed through Workers instead. I can't find any references to how it works for them in particular, but I believe that Memory { shared: false } are serializable and transferable, while Memory { shared: true } can only be shared across the agent cluster (not transferred or serialized, just like AB vs SAB).

With the implementation of that last PR, would passing WebAssembly.Memory objects work, or would that have to be done seperately?

@lucacasonato
Copy link
Member

It should work. WASM.Memory with shared: true are really SABs internally.

@lucacasonato
Copy link
Member

And we now have MessageChannel + MessagePort! Transferring them between workers has been broken out into #11074, and transferring them between BroadcastChannel into #11075

@lucacasonato
Copy link
Member

Message port transfer across workers is now implemented too. It will be in the next canary release in about 40 minutes (deno upgrade --canary).

@surma
Copy link
Contributor

surma commented Jun 22, 2021

Amazing. I’ll have a play! :D Thanks for your work on this, @lucacasonato!

@mhmdmar
Copy link

mhmdmar commented Jun 24, 2021

@lucacasonato I tried a simple script where I pass the port to a worker, the worker thread receives an empty object, I am using the latest version (deno 1.11.2+d54e3ea (canary, x86_64-pc-windows-msvc))

image

@mhmdmar
Copy link

mhmdmar commented Jun 24, 2021

Also I thought that you should know, when I try to pass a shared array buffer to the worker, I get a cloning error

image

@surma
Copy link
Contributor

surma commented Jun 24, 2021

@mhmdmar Looking at the code, you need to transfer the message port.

worker1.postMessage({port: port1}, [port1]);

(This is merely me looking at your JavaScript — I haven’t gotten around to testing this in Deno myself. Just as a heads up.)

@mhmdmar
Copy link

mhmdmar commented Jun 24, 2021

@surma This doesn't match the syntax in the browser, but even when I try this I still get the same behaviour, any ideas how to fix this ?

@lucacasonato
Copy link
Member

@mhmdmar It does match browser behaviour (message ports must always be included in the transfers array). You are likely running into bug #11102 though. As a workaround, get the port through the .ports array on the MessageEvent you receive in the worker.

@surma
Copy link
Contributor

surma commented Jun 24, 2021

Yeah you definitely need to transfer a MessagePort :D

Screen Shot 2021-06-24 at 19 00 33

@mhmdmar
Copy link

mhmdmar commented Jun 24, 2021

@lucacasonato Cool, I tried the workaround and it works, still even though I can get the port, I don't see the postMessage handler triggered from the parent

image

@surma
Copy link
Contributor

surma commented Jun 24, 2021

You need to listen on port2.onmessage if you are sending a message via port1.postMessage().

@mhmdmar
Copy link

mhmdmar commented Jun 24, 2021

@lucacasonato @surma Thank you both so much, it works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants