fix(ext/node) implement receiveMessageOnPort for node:worker_threads

[mash-graz]

Implementation of receiveMessageOnPort for node:worker_threads

Fixes: #22702

It's mostly reusing the code of the Web API MessagePort class, but adds a helper method to enable sync reading of entries in the message queue.

Although it's solving the main issue and mimics node behavior as good as possible, it's still limited by strictly utilizing the unmodified version of EventTarget as base class of MessagePort, while node is utilizing nodeEventTarget for EventErmitter backwards compatibility.

That's also the reason, why the node_compat test test-worker-message-port-receive-message.js doesn't work out of the box. I had to change one appearance of .on() to .addEventListener() at line 29 to bypass this obstacle.

I hope, it's correct to add this modified test in the ignore and tests section of config.jsonrc to prevent overwriting of my changes but also execution of this modified test, because it's still a rather useful compatibility check in regard to the receiveMessageOnPort operation.

Please check my suggested code changes carefully, because I'm not very familiar with deno internals.

[bartlomieju]

Looks good! Requested one change.

[bartlomieju]

MessagePort is a Web standard API and it doesn't have this method. We shouldn't add it. Instead use the body of this method in receiveMessageOnPort directly.

[mash-graz]

Yes -- I agree with you. I was trying to implement it in this way first, but I couldn't figure out any solution to get access to the private RID properties of the MessagePort class from an isolated function in node:worker_threads, where it should belong.

That's why I have chosen at the end this compromise, which looks at least more acceptable to me than leaking internal class data.

And this function is so close related and interwoven with all the MessageChannel and MessagePort internal infrastructure, that it somehow fits very well there.

Another solution could be perhaps a derived class -- similar to the nodeEventTarget construct -- to isolate the different specification contexts resp. their overlapping margins, but that's IMHO also not a really attractive solution.

Well -- I can change it in any way you like as long as it is technically possible. At the end it's just a matter of convincing arguments.

[mash-graz]

ps: Renaming and using a leading underscore would be another simple solution to clearly signify the proprietary nature of this method.

[bartlomieju]

You can export _id from 13_message_port.js and then import it in worker_threads.ts which will enable you to move all that receiveMessage logic into receiveMessageOnPort directly.

ps: Renaming and using a leading underscore would be another simple solution to clearly signify the proprietary nature of this method.

Once people find this API exist they will eventually start to rely on it and it will make it very hard to remove it, let's just make sure that all this fix is completely internal and doesn't expose any new methods and fields besides worker_threads::receiveMessageOnPort :)

[mash-graz]

You are right! -- I share your concerns.

I'll try to create a solution based on your suggestion (exporting the _id symbol) over the weekend.

[mash-graz]

I think, it should now look like you suggested.

[bartlomieju]

Yeah, this looks sensible. Let's see if CI passes.

[mash-graz]

I hope, this fixes the merge troubles and linters prefer-primordials warning.

[mash-graz]

There is now another important fix included in the last commit of this PR!

The logic for handling ESM files as worker source worked exactly the opposite way as expected!

I really would have liked to contribute this addition in a new PR, but the troubles already mentioned in the last comment of the still pending #22616 make everything rather uncomfortable and unnecessary tedious. :(

[bartlomieju]

There is now another important fix included in the last commit of this PR!

The logic for handling ESM files as worker source worked exactly the opposite way as expected!

I really would have liked to contribute this addition in a new PR, but the troubles already mentioned in the last comment of the still pending #22616 make everything rather uncomfortable and unnecessary tedious. :(

Great that you fixed it, but is there a test that you can enable or write yourself in tests/unit_node/worker_threads_test.ts? I can't really tell from looking at that change what's getting fixed.

To be clear, before the last commit the PR was okay and passed the CI - so it was ready to be merged. Maybe we could revert that last commit and do it in a separate PR?

[bartlomieju]

LGTM, great fix, thanks!

[mash-graz]

Thanks for merging! :)

I'll try to open a PR with the reverted fix, explanation of the issue and tests later...