fix(ext/node) implement receiveMessageOnPort for node:worker_threads #22766
Conversation
ext/web/13_message_port.js
Outdated
| receiveMessage() { | ||
| const data = op_message_port_recv_message_sync(this[_id]); | ||
| if (data === null) return undefined; | ||
| return { message: deserializeJsMessageData(data)[0] }; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ps: Renaming and using a leading underscore would be another simple solution to clearly signify the proprietary nature of this method.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
|
I think, it should now look like you suggested. |
|
Yeah, this looks sensible. Let's see if CI passes. |
|
I hope, this fixes the merge troubles and linters |
|
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 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? |
|
Thanks for merging! :) I'll try to open a PR with the reverted fix, explanation of the issue and tests later... |
Implementation of
receiveMessageOnPortfornode:worker_threadsFixes: #22702
It's mostly reusing the code of the Web API
MessagePortclass, but adds a helper method to enablesyncreading of entries in the message queue.Although it's solving the main issue and mimics
nodebehavior as good as possible, it's still limited by strictly utilizing the unmodified version ofEventTargetas base class ofMessagePort, whilenodeis utilizingnodeEventTargetforEventErmitterbackwards compatibility.That's also the reason, why the
node_compattesttest-worker-message-port-receive-message.jsdoesn'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
ignoreandtestssection ofconfig.jsonrcto prevent overwriting of my changes but also execution of this modified test, because it's still a rather useful compatibility check in regard to thereceiveMessageOnPortoperation.Please check my suggested code changes carefully, because I'm not very familiar with
denointernals.