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

doc: unclear conditions under which Worker's 'messageerror' is emmited #36333

Closed
1 task
naz opened this issue Dec 1, 2020 · 5 comments
Closed
1 task

doc: unclear conditions under which Worker's 'messageerror' is emmited #36333

naz opened this issue Dec 1, 2020 · 5 comments
Labels
doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support.

Comments

@naz
Copy link

naz commented Dec 1, 2020

  • Version: 14.15.1
  • Subsystem: worker_threads

Location

Section of the site where the content exists

Affected URL(s):

Description

Concise explanation of the problem

It is unclear from the documentation how one could cause/simulate messageerror to be emitted by the Worker. It would be useful to understand the conditions when it happens and also be able to simulate such situations when testing code. A concrete example when this would be helpful is in bree codebase, where test coverage has been skipped because there's no way to simulate this event.


  • I would like to work on this issue and
    submit a pull request.
@naz naz added the doc Issues and PRs related to the documentations. label Dec 1, 2020
@naz
Copy link
Author

naz commented Dec 1, 2020

To be clear, I'm happy to update the docs and provide a PR. Just lacking enough knowledge on the internal c++ workings of workers.

@Trott Trott added the worker Issues and PRs related to Worker support. label Dec 2, 2020
@Trott
Copy link
Member

Trott commented Dec 2, 2020

@nodejs/workers

@jasnell
Copy link
Member

jasnell commented Dec 7, 2020

pinging @addaleax (whom I believe has the most context / understanding of this part of the code)

@benjamingr
Copy link
Member

Look at parallel/test-crypto-key-objects-messageport.js and test/parallel/test-worker-message-port-transfer-filehandle.js

@addaleax
Copy link
Member

addaleax commented Dec 7, 2020

It is unclear from the documentation how one could cause/simulate messageerror to be emitted by the Worker.

Yeah, that’s a fair point. Part of why this was was left appear unclear is that it is also unclear (to me) in the HTML spec when this event would be emitted in the browser implementations.

Currently, this event is emitted when there is an error occuring while instantiating the posted JS object on the receiving end, where there would otherwise be no way to communicate that situation.

The examples pointed to by @benjamingr are for situations in which Node.js API objects (not JS built-ins) are received in a vm.Context, which is currently an environment in which Node.js APIs are not available. That is a known limitation for now, rather than something that is inherent to the way we build APIs. It’s also not a condition under which messageerror would be emitted on a Worker, only MessagePorts (because there’s no way to create a Worker inside a vm.Context).

You could probably make some of the deserialization methods throw an exception in some way, and generate the event through that, for example by messing with the prototypes of Node.js API objects, but ultimately, I don’t think there’s any good way to generate these events through the public API currently.

I can’t think of any reason why deserializing a message should fail once it has been posted, except for limitations in Node.js’ builtins, so, for testing, I would currently recommend just to emit the events directly.

jasnell added a commit to jasnell/node that referenced this issue Jan 4, 2021
Adapting addaleax's explanation from the issue.

Fixes: nodejs#36333
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell closed this as completed in 9ac4047 Jan 6, 2021
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Adapting addaleax's explanation from the issue.

Fixes: #36333
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36780
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
targos pushed a commit that referenced this issue May 1, 2021
Adapting addaleax's explanation from the issue.

Fixes: #36333
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36780
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants