nerror
nerror copied to clipboard
feat: support upcasting into VError
Motivation
Throughout our application stack(s) we rely on and expect VErrors as well as some of the methods associated with them. Often times libraries (or Node core) generate standard Errors that we'd like to use VError semantics with. We'd like to be able to do this without re-wrapping them as a VError cause, or re-creating a brand new error and lifting over things like err.stack
which might make core file analysis more difficult.
Sample scenario:
const req = http.request('http://1.2.3.4/foo');
req.on('error', function(err) {
// err here is a 'socket hang up'!
// how can we add context to this error without creating a new empty VError wrapper?
});
This PR introduces a static method VError.upcast(err)
which initializes the target error with the necessary internal VError fields and methods at the instance level. It does not muck with prototypes or classes of the target error.
Feedback I'm looking for
- I'm not sure if we should call this
upcast
.mixin
might be more appropriate, though that word may have more baggage associated with it? - Any debugging concerns with extending or touching errors being generated or created by other libraries?
- Any performance gotchas or V8 gotchas I should be concerned with?
cc @kurisuchan @daviande
As I know the best practice with normal errors are: new Error(err) where err can be an instanceof Error or string or pretty much anything. Why not to advocate the same or can we overwrite/keep the original stack trace or just use cause?
As I remember VError already sets cause if constructor argument is an Error.
@DonutEspresso Can you elaborate on:
make core file analysis more difficult.
? Is it that:
- when one finds an instance of VError in a core dump, then they have to follow the chain of causes manually to the first one
- only then they can get the actual callstack of where that error was created
- the whole process is manual and tedious
?
Also, what if I upcast the same error twice and on the second upcast I call e.g. assignInfo
for a key that was already set during the first upcast? In other words, it seems one of the nice properties of wrapping errors with a new Error instance is that one doesn't have to know whether it's safe to mutate the original error.
Is it that:
- when one finds an instance of VError in a core dump, then they have to follow the chain of causes manually to the first one
- only then they can get the actual callstack of where that error was created
- the whole process is manual and tedious
Sorry, I probably wasn't super clear with that problem statement. I am likely being overly inclusive by saying we want to enable the full breadth of VError semantics to standard errors. For example, I can't imagine a scenario where you want to add a cause to an existing error that was bubbled up to you.
The goal is to enable us to use assignInfo
or add info
(jse_info
) to non-VError errors. To enable using info
on non-VError errors, we have a few options:
- Wrap the error with an "empty" VError using the
cause
argument, adding info to this VError wrapper - Create a new VError, lift over attributes like
stack
andmsg
from the original error (this is what I had concerns about re: core analysis, as I'm wondering if we would lose some fidelity in the core file by doing a sloppy "copy" of an error) - Add VError semantics to the error without creating a new Error object in any form (the current
upcast
PR proposal).
The last option seemed to bring the most consistency for libraries dealing with non-VError errors. This would probably be frequently used by any module building on top of core modules (e.g., restify-clients). But I'm not sure I'm sold on this being the right approach yet.