src: improve error handling in buffer and dotenv #57189
Conversation
nodejs-github-bot
added
c++
| } | ||
|
|
||
| Local<Object> Dotenv::ToObject(Environment* env) const { | ||
| MaybeLocal<Object> Dotenv::ToObject(Environment* env) const { | ||
| EscapableHandleScope scope(env->isolate()); |
There was a problem hiding this comment.
For educational purposes: why is this needed?
There was a problem hiding this comment.
Any JS handles that are created while this is running will be created within whatever handlescope is on the stack. If this method errors, those handles will end up being kept around until that outer scope exits. This ensures that in the error case, any handles created during this function are cleaned up immediately.
There was a problem hiding this comment.
Why do we not have this everywhere?
| @@ -511,7 +511,7 @@ static void LoadEnvFile(const v8::FunctionCallbackInfo<v8::Value>& args) { | |||
|
|
|||
| switch (dotenv.ParsePath(path)) { | |||
| case dotenv.ParseResult::Valid: { | |||
| dotenv.SetEnvironment(env); | |||
| USE(dotenv.SetEnvironment(env)); | |||
There was a problem hiding this comment.
Shouldn't we just throw here, rather than omitting the error?
There was a problem hiding this comment.
We're not ignoring the error. In this branch we end up immediately breaking and skipping to the end of the function, allowing the error to propagate.
This comment was marked as outdated.
This comment was marked as outdated.
jasnell
force-pushed
the
jasnell/improve-errorhandling-multiple
branch
from
d0e8328 to
9d16f80
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Replacing ToLocalChecked()
jasnell
force-pushed
the
jasnell/improve-errorhandling-multiple
branch
from
9d16f80 to
c8d7c8d
Compare
jasnell
added
the
author ready
Replacing ToLocalChecked()