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: serialize errors if stack getter throws #26145

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 16, 2019

Remove an apparently unnecessary try/catch from the error
serialization/deserialization internal module used by worker_threads.

(If I'm wrong about this being unnecessary for some reason, I'll switch course and try to add a test and/or an explanatory comment. But it sure seems to effectively be a no-op?)

Current code that is intended to handle the stack getter throwing is untested. Add a test and adjust code to function as expected.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott Trott requested a review from addaleax February 16, 2019 06:46
@Trott
Copy link
Member Author

Trott commented Feb 16, 2019

@Trott Trott added errors Issues and PRs related to JavaScript errors originated in Node.js core. worker Issues and PRs related to Worker support. labels Feb 16, 2019
@addaleax
Copy link
Member

The idea behind this line was that .stack is implemented as a “hidden” getter on the V8 side, which can call into user code via Error.prepareStackTrace (if that is defined), which in turn might throw an error itself. (Probably should have left a comment for that. :))

I don’t think it would be bad to fall back back to the second case (kSerializedObject) in such a situation, though – it wouldn’t be very different from a getter on a nested property of the error object throwing an error.

@Trott
Copy link
Member Author

Trott commented Feb 19, 2019

@addaleax I'm having difficulty finding a way to show this line having an effect. Here's the test I came up with:

'use strict';
const common = require('../common');
const assert = require('assert');
const { Worker } = require('worker_threads');

const w = new Worker(`
  const fn = (err) => {
    if (err.message === 'fhqwhgads')
      throw new Error('come on');
    return 'This is my custom stack trace!';
  };
  Error.prepareStackTrace = fn;
  throw new Error('fhqwhgads');
  `,
  { eval: true }
);
w.on('message', common.mustNotCall());
w.on('error', common.mustCall((err) => {
  console.log(err);
}));

But whether I compile with or without the line removed in this PR, the output is the same:

Error [ERR_WORKER_UNSERIALIZABLE_ERROR]: Serializing an uncaught exception failed
    at Worker.[kOnCouldNotSerializeErr] (internal/worker.js:135:24)
    at Worker.[kOnMessage] (internal/worker.js:149:45)
    at MessagePort.Worker.(anonymous function).on (internal/worker.js:84:57)
    at MessagePort.emit (events.js:197:13)
    at MessagePort.onmessage (internal/worker/io.js:62:8)

Do I need to revise the test? Or does this show that the line has no effect either way?

@addaleax
Copy link
Member

@Trott Yeah, I get the same results. It looks like V8 tries to call prepareStackTrace again if it failed the first time? When adding another try/catch like this, I do get the serialized exception with your test:

diff --git a/lib/internal/error-serdes.js b/lib/internal/error-serdes.js
index 7b4b416b80c6..bba49c782fff 100644
--- a/lib/internal/error-serdes.js
+++ b/lib/internal/error-serdes.js
@@ -36,7 +36,10 @@ function TryGetAllProperties(object, target = object) {
   Assign(all, TryGetAllProperties(GetPrototypeOf(object), target));
   const keys = GetOwnPropertyNames(object);
   ForEach(keys, (key) => {
-    const descriptor = GetOwnPropertyDescriptor(object, key);
+    let descriptor;
+    try {
+      descriptor = GetOwnPropertyDescriptor(object, key);
+    } catch { return; }
     const getter = descriptor.get;
     if (getter && key !== '__proto__') {
       try {

Current code that is intended to handle the stack getter throwing is
untested. Add a test and adjust code to function as expected.

Co-authored-by: Anna Henningsen <[email protected]>
@Trott
Copy link
Member Author

Trott commented Feb 20, 2019

OK, added the test, and adjusted the code per @addaleax's comment (and added a Co-Authored-By for her). PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/20899/ (:heavy_check_mark:)

@Trott Trott changed the title worker: remove unnecessary try/catch worker: serialize errors if stack getter throws Feb 20, 2019
@addaleax
Copy link
Member

Thanks!

@addaleax
Copy link
Member

@devsnek You probably want to re-review this :)

let descriptor;
try {
descriptor = GetOwnPropertyDescriptor(object, key);
} catch { return; }
Copy link
Member

Choose a reason for hiding this comment

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

I am fine with returning undefined but wouldn't it be even better to return the error to the user as well? We could wrap it into something else so it's clear that accessing the property threw (for example by adding a new Node.js error like ERR_THREW_ON_ACCESS) which contains the actual error as property.

assert.strictEqual(err.stack, undefined);
assert.strictEqual(err.message, 'fhqwhgads');
assert.strictEqual(err.name, 'Error');
}));
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to change this part to:

w.on('error', common.expectsError({
  stack: undefined,
  message: 'fhqwhgads',
  name: 'Error'
});

@devsnek
Copy link
Member

devsnek commented Feb 20, 2019

i am confused why we wouldn't just let the error bubble up. the entire state of the resulting object could be silently incorrect because of this.

@Trott
Copy link
Member Author

Trott commented Feb 20, 2019

i am confused why we wouldn't just let the error bubble up. the entire state of the resulting object could be silently incorrect because of this.

I'm fine with that. How about you, @addaleax?

Error [ERR_WORKER_UNSERIALIZABLE_ERROR]: Serializing an uncaught exception failed
    at Worker.[kOnCouldNotSerializeErr] (internal/worker.js:135:24)
    at Worker.[kOnMessage] (internal/worker.js:149:45)
    at MessagePort.Worker.(anonymous function).on (internal/worker.js:84:57)
    at MessagePort.emit (events.js:197:13)
    at MessagePort.onmessage (internal/worker/io.js:62:8)

@addaleax
Copy link
Member

@Trott @devsnek It’s important to me that some helpful information about the original error survives, and ideally as much as possible. It’s not clear to me how “Serializing an uncaught exception failed” is better than providing an object without a stack trace (which is, admittedly, already pretty unhelpful).

@Trott
Copy link
Member Author

Trott commented Feb 21, 2019

@Trott @devsnek It’s important to me that some helpful information about the original error survives, and ideally as much as possible. It’s not clear to me how “Serializing an uncaught exception failed” is better than providing an object without a stack trace (which is, admittedly, already pretty unhelpful).

So that then brings us to @BridgeAR's suggestion of trying to provide some helpful information when it throws.

Thinking about it more, I would most likely prefer that we emulate as closely as possible what happens outside of workers so that at least people can debug and not be confused that they get one error outside of workers and a different error in a worker. Here's what happens outside of a worker:

/Users/trott/io.js/test/parallel/test-worker-error-stack-getter-throws.js:30
throw new Error('fhqwhgads');
^

Error: fhqwhgads

So that would argue for the approach currently here where we return the error with no stack trace at all and no indication of an additional error while creating the stack trace. @devsnek?

@devsnek
Copy link
Member

devsnek commented Feb 21, 2019

maybe i'm just missing something obvious here, but why isn't this diff just

-try { error.stack; } catch {}

and nothing else? just let the error bubble up to the serialize() call

@Trott
Copy link
Member Author

Trott commented Feb 21, 2019

@devsnek That's what it was initially but doing that results in this error in the test:

Error [ERR_WORKER_UNSERIALIZABLE_ERROR]: Serializing an uncaught exception failed
    at Worker.[kOnCouldNotSerializeErr] (internal/worker.js:135:24)
    at Worker.[kOnMessage] (internal/worker.js:149:45)
    at MessagePort.Worker.(anonymous function).on (internal/worker.js:84:57)
    at MessagePort.emit (events.js:197:13)
    at MessagePort.onmessage (internal/worker/io.js:62:8)

That's OK, I guess, but it has the following shortcomings:

  • The end user has no idea what the original error is.
  • The end user has no idea what the additional error thrown by the stack getter was, or even that there was an additional error thrown by the stack getter.
  • It is very different from what happens with the same code run outside of a worker.

What's here now may be less-than-ideal, but it has the benefits of:

  • Being closely aligned with what happens when the code is run outside of a worker.
  • Reporting the original error.

@devsnek
Copy link
Member

devsnek commented Feb 21, 2019

wherever ERR_WORKER_UNSERIALIZABLE_ERROR is thrown we could just re-throw the actual error right?

i'd expect

try {
  postMessage({
    get a() {
      throw 'xyz';
    },
  });
} catch (e) {
  console.log(e);
}

to print 'xyz', just like it does in browser.

@addaleax
Copy link
Member

wherever ERR_WORKER_UNSERIALIZABLE_ERROR is thrown we could just re-throw the actual error right?

@devsnek Can you clarify what you mean by “re-throw”? This is coming from the uncaught exception handler, not from a try/catch.

to print 'xyz', just like it does in browser.

That’s already happening here as well.

@devsnek
Copy link
Member

devsnek commented Feb 24, 2019

ok i see how this code is actually being used now. i would probably just copy what browsers do and ignore the stack property.

https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2

https://repl.it/repls/ForthrightWhoppingParentheses

@addaleax
Copy link
Member

@devsnek I think the stack trace is too valuable to be omitted. I honestly wouldn’t know what to do if I didn’t have it available when debugging the worker tests. :)

@Trott
Copy link
Member Author

Trott commented Feb 25, 2019

@devsnek I can't tell if your comments are comments/discussion, or if you're -1 on landing this. Can you let me know?

@devsnek
Copy link
Member

devsnek commented Feb 25, 2019

@Trott i'm not blocking this

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 25, 2019
@Trott
Copy link
Member Author

Trott commented Feb 25, 2019

Landed in 79a3348

Trott added a commit to Trott/io.js that referenced this pull request Feb 25, 2019
Current code that is intended to handle the stack getter throwing is
untested. Add a test and adjust code to function as expected.

Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: nodejs#26145
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott Trott closed this Feb 25, 2019
addaleax added a commit that referenced this pull request Feb 25, 2019
Current code that is intended to handle the stack getter throwing is
untested. Add a test and adjust code to function as expected.

Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #26145
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Current code that is intended to handle the stack getter throwing is
untested. Add a test and adjust code to function as expected.

Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #26145
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott Trott deleted the remove-try-catch branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants