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

Fix 6899: console.* not working when running out of stack space #6907

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@addaleax
Copy link
Member

commented May 20, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
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/

@bnoordhuis

View changes

src/node_contextify.cc Outdated
@@ -58,6 +58,9 @@ class ContextifyContext {
public:
ContextifyContext(Environment* env, Local<Object> sandbox_obj) : env_(env) {
Local<Context> v8_context = CreateV8Context(env, sandbox_obj);
if (v8_context.IsEmpty())

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 21, 2016

Member

The context_.IsEmpty() check below already takes care of this. persistent_handle.Reset(isolate, empty_handle) is functionally equivalent to persistent_handle.Reset().

This comment has been minimized.

Copy link
@addaleax

addaleax May 21, 2016

Author Member

@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.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 21, 2016

Member

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

This comment has been minimized.

Copy link
@addaleax

addaleax May 21, 2016

Author Member

@bnoordhuis Sorry, you’re right, I must have had something else in mind…

@bnoordhuis

View changes

src/node_contextify.cc Outdated
CHECK(!ctx.IsEmpty());
if (ctx.IsEmpty()) {
env->ThrowError("Could not instantiate context");
return ctx;

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 21, 2016

Member

Not wrong but return Local<Context>() makes it clear without, ah, context that we're returning an empty handle.

@bnoordhuis

View changes

src/node_contextify.cc Outdated
@@ -634,7 +641,7 @@ class ContextifyScript : public BaseObject {
Local<Value> arrow;
if (!(maybe_value.ToLocal(&arrow) &&
arrow->IsString() &&
stack->IsString())) {
!stack.IsEmpty() && stack->IsString())) {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 21, 2016

Member

I'd maybe break this out into separate if statements for clarity (double negation is hard to parse for humans) but I'll leave that up to you.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 21, 2016

LGTM, by the way.

@addaleax addaleax force-pushed the addaleax:fix-6899 branch 2 times, most recently May 21, 2016

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 21, 2016

Updated based on @bnoordhuis’ suggestions.

New CI: https://ci.nodejs.org/job/node-test-commit/3428/

@bnoordhuis

View changes

src/node_contextify.cc Outdated
@@ -58,6 +58,7 @@ class ContextifyContext {
public:
ContextifyContext(Environment* env, Local<Object> sandbox_obj) : env_(env) {
Local<Context> v8_context = CreateV8Context(env, sandbox_obj);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 22, 2016

Member

Whitespace change.

@bnoordhuis

View changes

src/node_contextify.cc Outdated
@@ -633,8 +638,11 @@ class ContextifyScript : public BaseObject {

Local<Value> arrow;
if (!(maybe_value.ToLocal(&arrow) &&
arrow->IsString() &&
stack->IsString())) {
arrow->IsString())) {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 22, 2016

Member

Fits on one line.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 22, 2016

LGTM with style nits. The CI failures are probably all flakes but you may want to rerun it, just in case.

addaleax added some commits May 20, 2016

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.

Ref: #6899
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.

Fixes: #6899

@addaleax addaleax force-pushed the addaleax:fix-6899 branch to 7ad588e May 22, 2016

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 22, 2016

Addressed nits… CI rerun (although I think some of the failures were infrastructure-related): https://ci.nodejs.org/job/node-test-commit/3435/

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 24, 2016

Landed in 4bd410b, dab0987

@addaleax addaleax closed this May 24, 2016

@addaleax addaleax deleted the addaleax:fix-6899 branch May 24, 2016

addaleax added a commit that referenced this pull request May 24, 2016

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.

Ref: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax added a commit that referenced this pull request May 24, 2016

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.

Fixes: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 30, 2016

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.

Ref: nodejs#6899
PR-URL: nodejs#6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 30, 2016

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.

Fixes: nodejs#6899
PR-URL: nodejs#6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg added a commit that referenced this pull request Jun 2, 2016

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.

Ref: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg added a commit that referenced this pull request Jun 2, 2016

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.

Fixes: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

@addaleax lts?

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2016

@thealphanerd Backport is linked above (#6957) :)

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

OH YEAH... I was waiting for it to live in a release... this is what I get for doing things on autopilot

MylesBorins added a commit that referenced this pull request Jun 30, 2016

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.

Ref: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jun 30, 2016

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.

Fixes: #6899

Ref: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

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.

Ref: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

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.

Fixes: #6899

Ref: #6899
PR-URL: #6907
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Jul 12, 2016

Merged

v4.5.0 proposal #7688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.