Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Mar 8, 2022

Fix library_pthread.js onmessage error detection. #16239 (comment)

@merceyz
Copy link

merceyz commented Mar 8, 2022

Probably not for this PR but it should be possible to move the "internal" communication to its own MessageChannel so it doesn't interfere with whatever a user might be sending.

} else if (cmd) {
// The received message looks like something that should be handled by this message
// handler, (since there is a e.data.cmd field present), but is not one of the
// recognized commands:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't the worker itself and implementation detail of the pthread library?

Are we suggesting that users should be able to directly access the worker object and attach their own message handlers to it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently have a mechanism in place to distinguish between public and private symbols. It is not surprising to see users directly access PThread.pthreads[id].worker field themselves. This is similar to the other objects (GL, Browser, JSEvents) that people generally directly access to manipulate whatever they need to integrate with from the JS side.

None of that has been given any guarantees to stay stable, but that has not stopped people.

That being said, I think it would be a good idea to have a stable API to access the Worker object of a pthread from JS. It is possible that there are e.g. existing JS libraries out there that deal with registering message event listeners in Worker scope, either via onmessage or addEventListener, so not preventing those from working would be a good idea (especially given that the requirement to do so is next to nothing)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. We can land this now, and look into added a more official API, although with an example of how to do this.. but happy for that to be a followup.

@juj juj enabled auto-merge (squash) March 9, 2022 16:44
@juj juj merged commit 504f4e0 into emscripten-core:main Mar 9, 2022
kripken pushed a commit that referenced this pull request Sep 16, 2022
Mirrors the fix from #16450 which solves the issue for library_pthread.js to fix worker.js as well.

Fixes #17844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants