Fix 6899: console.* not working when running out of stack space - #6907
Fix 6899: console.* not working when running out of stack space#6907addaleax wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
The context_.IsEmpty() check below already takes care of this. persistent_handle.Reset(isolate, empty_handle) is functionally equivalent to persistent_handle.Reset().
Sorry, something went wrong.
There was a problem hiding this comment.
@bnoordhuis I won’t be able to take care of that until at least tomorrow… could you double-check that? I was actually getting a fatal error due to this, and I was surprised that the check below didn’t catch that and this one did.
Sorry, something went wrong.
There was a problem hiding this comment.
Works for me locally.
EDIT: Just so we're talking about the same thing:
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index ae88223..2b23c08 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -58,9 +58,6 @@ class ContextifyContext {
public:
ContextifyContext(Environment* env, Local<Object> sandbox_obj) : env_(env) {
Local<Context> v8_context = CreateV8Context(env, sandbox_obj);
- if (v8_context.IsEmpty())
- return;
-
context_.Reset(env->isolate(), v8_context);
// Allocation failure or maximum call stack size reached
Sorry, something went wrong.
There was a problem hiding this comment.
@bnoordhuis Sorry, you’re right, I must have had something else in mind…
Sorry, something went wrong.
|
LGTM, by the way. |
Sorry, something went wrong.
|
Updated based on @bnoordhuis’ suggestions. |
Sorry, something went wrong.
There was a problem hiding this comment.
Whitespace change.
Sorry, something went wrong.
|
LGTM with style nits. The CI failures are probably all flakes but you may want to rerun it, just in case. |
Sorry, something went wrong.
Make less assumptions about what objects will be available when vm context creation or error message printing fail because V8 runs out of JS stack space. Ref: nodejs#6899
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional `loading` flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value. This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point. Fixes: nodejs#6899
|
Addressed nits… CI rerun (although I think some of the failures were infrastructure-related): https://ci.nodejs.org/job/node-test-commit/3435/ |
Sorry, something went wrong.
Sorry, something went wrong.
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional `loading` flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value. This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point. Fixes: #6899 PR-URL: #6907 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Make less assumptions about what objects will be available when vm context creation or error message printing fail because V8 runs out of JS stack space. Ref: nodejs#6899 PR-URL: nodejs#6907 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional `loading` flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value. This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point. Fixes: nodejs#6899 PR-URL: nodejs#6907 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional `loading` flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value. This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point. Fixes: #6899 PR-URL: #6907 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
@thealphanerd Backport is linked above (#6957) :) |
Sorry, something went wrong.
|
OH YEAH... I was waiting for it to live in a release... this is what I get for doing things on autopilot |
Sorry, something went wrong.
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional `loading` flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value. This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point. Fixes: #6899 Ref: #6899 PR-URL: #6907 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional `loading` flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value. This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point. Fixes: #6899 Ref: #6899 PR-URL: #6907 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Checklist
Affected core subsystem(s)
vm, module(?), console(?)
Description of change
Commit 1: vm: don't abort process when stack space runs out
Make less assumptions about what objects will be available when vm context creation or error message printing fail because V8 runs out of JS stack space.
Commit 2: module: don't cache uninitialized builtins
Don't cache the exported values of fully uninitialized builtins. This works by adding an additional loading flag that is only active during initial loading of an internal module and checking that either the module is fully loaded or is in that state before using its cached value.
This has the effect that builtins modules which could not be loaded (e.g. because compilation failed due to missing stack space) can be loaded at a later point.
I’m definitely open for the possibility of leaving the module commit out, if others think this specific bug is not worth fixing beyond not aborting the process.
Initial CI: https://ci.nodejs.org/job/node-test-commit/3417/