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

Abort when using --trace-events-enabled on test-http2-respond-with-file #19921

Closed
jasnell opened this issue Apr 10, 2018 · 5 comments
Closed

Comments

@jasnell
Copy link
Member

jasnell commented Apr 10, 2018

$ ./node --trace-events-enabled test/parallel/test-http2-respond-file
Warning: Trace event is an experimental feature and could change at any time.
(node:79696) ExperimentalWarning: The http2 module is an experimental API.
FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
 1: node::Abort() [../node/node/node]
 2: 0x5637918a55ae [../node/node/node]
 3: v8::Utils::ReportApiFailure(char const*, char const*) [../node/node/node]
 4: v8::internal::HandleScope::Extend(v8::internal::Isolate*) [../node/node/node]
 5: v8::internal::Factory::NewNumber(double, v8::internal::PretenureFlag) [../node/node/node]
 6: v8::Number::New(v8::Isolate*, double) [../node/node/node]
 7: node::AsyncWrap::EmitBefore(node::Environment*, double) [../node/node/node]
 8: node::StreamPipe::ReadableListener::OnStreamRead(long, uv_buf_t const&) [../node/node/node]
 9: 0x5637918df586 [../node/node/node]
10: 0x5637919b4334 [../node/node/node]
11: 0x5637919b6b12 [../node/node/node]
12: 0x5637919c996a [../node/node/node]
13: uv_run [../node/node/node]
14: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [../node/node/node]
15: node::Start(int, char**) [../node/node/node]
16: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
17: _start [../node/node/node]
Aborted (core dumped)

ping @addaleax @AndreasMadsen @nodejs/diagnostics

@jasnell
Copy link
Member Author

jasnell commented Apr 10, 2018

@addaleax ... appears to happen only with the respond-with-file tests

@addaleax
Copy link
Member

Adding the handle scopes should be enough, I think?

diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h
index c9f123332430..f9c709aa21e9 100644
--- a/src/async_wrap-inl.h
+++ b/src/async_wrap-inl.h
@@ -50,6 +50,7 @@ inline AsyncWrap::AsyncScope::AsyncScope(AsyncWrap* wrap)
   Environment* env = wrap->env();
   if (env->async_hooks()->fields()[Environment::AsyncHooks::kBefore] == 0)
     return;
+  v8::HandleScope handle_scope(env->isolate());
   EmitBefore(env, wrap->get_async_id());
 }
 
@@ -57,6 +58,7 @@ inline AsyncWrap::AsyncScope::~AsyncScope() {
   Environment* env = wrap_->env();
   if (env->async_hooks()->fields()[Environment::AsyncHooks::kAfter] == 0)
     return;
+  v8::HandleScope handle_scope(env->isolate());
   EmitAfter(env, wrap_->get_async_id());
 }
 

@jasnell
Copy link
Member Author

jasnell commented Apr 10, 2018

I think so, yes :-)

@ryzokuken
Copy link
Contributor

@addaleax @jasnell do the above changes make it work perfectly? Should I submit a patch with them?

@jasnell
Copy link
Member Author

jasnell commented Apr 11, 2018

I haven't checked yet but it's simple to test :-)

ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 12, 2018
Add `HandleError`s to the AsyncScope constructor and destructor in
async_wrap-inl.h to fix "FATAL ERROR" incurring observed while running
test-http2-respond-with-file using the --trace-events-enabled flag.

Fixes: nodejs#19921
jasnell pushed a commit that referenced this issue Apr 16, 2018
Add `HandleError`s to the AsyncScope constructor and destructor in
async_wrap-inl.h to fix "FATAL ERROR" incurring observed while running
test-http2-respond-with-file using the --trace-events-enabled flag.

Fixes: #19921

PR-URL: #19972
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants