Skip to content

Commit

Permalink
async_wrap: call destroy() callback in uv_idle_t
Browse files Browse the repository at this point in the history
Calling JS during GC is a no-no. So intead create a queue of all ids
that need to have their destroy() callback called and call them later.

Removed checking destroy() in test-async-wrap-uid because destroy() can
be called after the 'exit' callback.

Missing a reliable test to reproduce the issue that caused the
FATAL_ERROR.

PR-URL: #10096
Fixes: #8216
Fixes: #9465
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
trevnorris authored and MylesBorins committed Dec 6, 2016
1 parent 3e5b2eb commit f3b0cf5
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 25 deletions.
49 changes: 37 additions & 12 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "util.h"
#include "util-inl.h"

#include "uv.h"
#include "v8.h"
#include "v8-profiler.h"

Expand Down Expand Up @@ -182,6 +183,38 @@ void AsyncWrap::Initialize(Local<Object> target,
}


void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) {
uv_idle_stop(handle);

Environment* env = Environment::from_destroy_ids_idle_handle(handle);
// None of the V8 calls done outside the HandleScope leak a handle. If this
// changes in the future then the SealHandleScope wrapping the uv_run()
// will catch this can cause the process to abort.
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());
Local<Function> fn = env->async_hooks_destroy_function();

if (fn.IsEmpty())
return env->destroy_ids_list()->clear();

TryCatch try_catch(env->isolate());

for (auto current_id : *env->destroy_ids_list()) {
// Want each callback to be cleaned up after itself, instead of cleaning
// them all up after the while() loop completes.
HandleScope scope(env->isolate());
Local<Value> argv = Number::New(env->isolate(), current_id);
MaybeLocal<Value> ret = fn->Call(
env->context(), Undefined(env->isolate()), 1, &argv);

if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
}
}
}


void LoadAsyncWrapperInfo(Environment* env) {
HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler();
#define V(PROVIDER) \
Expand Down Expand Up @@ -248,18 +281,10 @@ AsyncWrap::~AsyncWrap() {
if (!ran_init_callback())
return;

Local<Function> fn = env()->async_hooks_destroy_function();
if (!fn.IsEmpty()) {
HandleScope scope(env()->isolate());
Local<Value> uid = Number::New(env()->isolate(), get_uid());
TryCatch try_catch(env()->isolate());
MaybeLocal<Value> ret =
fn->Call(env()->context(), Null(env()->isolate()), 1, &uid);
if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
}
}
if (env()->destroy_ids_list()->empty())
uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb);

env()->destroy_ids_list()->push_back(get_uid());
}


Expand Down
3 changes: 3 additions & 0 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "base-object.h"
#include "uv.h"
#include "v8.h"

#include <stdint.h>
Expand Down Expand Up @@ -60,6 +61,8 @@ class AsyncWrap : public BaseObject {
v8::Local<v8::Value> unused,
v8::Local<v8::Context> context);

static void DestroyIdsCb(uv_idle_t* handle);

inline ProviderType provider_type() const;

inline int64_t get_uid() const;
Expand Down
15 changes: 15 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ inline Environment::Environment(v8::Local<v8::Context> context,

RB_INIT(&cares_task_list_);
handle_cleanup_waiting_ = 0;

destroy_ids_list_.reserve(512);
}

inline Environment::~Environment() {
Expand Down Expand Up @@ -305,6 +307,15 @@ inline uv_idle_t* Environment::immediate_idle_handle() {
return &immediate_idle_handle_;
}

inline Environment* Environment::from_destroy_ids_idle_handle(
uv_idle_t* handle) {
return ContainerOf(&Environment::destroy_ids_idle_handle_, handle);
}

inline uv_idle_t* Environment::destroy_ids_idle_handle() {
return &destroy_ids_idle_handle_;
}

inline Environment* Environment::from_idle_prepare_handle(
uv_prepare_t* handle) {
return ContainerOf(&Environment::idle_prepare_handle_, handle);
Expand Down Expand Up @@ -381,6 +392,10 @@ inline int64_t Environment::get_async_wrap_uid() {
return ++async_wrap_uid_;
}

inline std::vector<int64_t>* Environment::destroy_ids_list() {
return &destroy_ids_list_;
}

inline uint32_t* Environment::heap_statistics_buffer() const {
CHECK_NE(heap_statistics_buffer_, nullptr);
return heap_statistics_buffer_;
Expand Down
8 changes: 8 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "v8.h"

#include <stdint.h>
#include <vector>

// Caveat emptor: we're going slightly crazy with macros here but the end
// hopefully justifies the means. We have a lot of per-context properties
Expand Down Expand Up @@ -413,8 +414,10 @@ class Environment {
inline uint32_t watched_providers() const;

static inline Environment* from_immediate_check_handle(uv_check_t* handle);
static inline Environment* from_destroy_ids_idle_handle(uv_idle_t* handle);
inline uv_check_t* immediate_check_handle();
inline uv_idle_t* immediate_idle_handle();
inline uv_idle_t* destroy_ids_idle_handle();

static inline Environment* from_idle_prepare_handle(uv_prepare_t* handle);
inline uv_prepare_t* idle_prepare_handle();
Expand Down Expand Up @@ -451,6 +454,9 @@ class Environment {

inline int64_t get_async_wrap_uid();

// List of id's that have been destroyed and need the destroy() cb called.
inline std::vector<int64_t>* destroy_ids_list();

inline uint32_t* heap_statistics_buffer() const;
inline void set_heap_statistics_buffer(uint32_t* pointer);

Expand Down Expand Up @@ -543,6 +549,7 @@ class Environment {
IsolateData* const isolate_data_;
uv_check_t immediate_check_handle_;
uv_idle_t immediate_idle_handle_;
uv_idle_t destroy_ids_idle_handle_;
uv_prepare_t idle_prepare_handle_;
uv_check_t idle_check_handle_;
AsyncHooks async_hooks_;
Expand All @@ -558,6 +565,7 @@ class Environment {
bool trace_sync_io_;
size_t makecallback_cntr_;
int64_t async_wrap_uid_;
std::vector<int64_t> destroy_ids_list_;
debugger::Agent debugger_agent_;
#if HAVE_INSPECTOR
inspector::Agent inspector_agent_;
Expand Down
3 changes: 3 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4511,6 +4511,9 @@ Environment* CreateEnvironment(Isolate* isolate,
uv_unref(reinterpret_cast<uv_handle_t*>(env->idle_prepare_handle()));
uv_unref(reinterpret_cast<uv_handle_t*>(env->idle_check_handle()));

uv_idle_init(env->event_loop(), env->destroy_ids_idle_handle());
uv_unref(reinterpret_cast<uv_handle_t*>(env->destroy_ids_idle_handle()));

// Register handle cleanups
env->RegisterHandleCleanup(
reinterpret_cast<uv_handle_t*>(env->immediate_check_handle()),
Expand Down
8 changes: 2 additions & 6 deletions test/parallel/test-async-wrap-throw-from-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const assert = require('assert');
const crypto = require('crypto');
const domain = require('domain');
const spawn = require('child_process').spawn;
const callbacks = [ 'init', 'pre', 'post', 'destroy' ];
const callbacks = [ 'init', 'pre', 'post' ];
const toCall = process.argv[2];
var msgCalled = 0;
var msgReceived = 0;
Expand All @@ -23,13 +23,9 @@ function post() {
if (toCall === 'post')
throw new Error('post');
}
function destroy() {
if (toCall === 'destroy')
throw new Error('destroy');
}

if (typeof process.argv[2] === 'string') {
async_wrap.setupHooks({ init, pre, post, destroy });
async_wrap.setupHooks({ init, pre, post });
async_wrap.enable();

process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE'));
Expand Down
8 changes: 1 addition & 7 deletions test/parallel/test-async-wrap-uid.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,14 @@ const assert = require('assert');
const async_wrap = process.binding('async_wrap');

const storage = new Map();
async_wrap.setupHooks({ init, pre, post, destroy });
async_wrap.setupHooks({ init, pre, post });
async_wrap.enable();

function init(uid) {
storage.set(uid, {
init: true,
pre: false,
post: false,
destroy: false
});
}

Expand All @@ -26,10 +25,6 @@ function post(uid) {
storage.get(uid).post = true;
}

function destroy(uid) {
storage.get(uid).destroy = true;
}

fs.access(__filename, function(err) {
assert.ifError(err);
});
Expand All @@ -51,7 +46,6 @@ process.once('exit', function() {
init: true,
pre: true,
post: true,
destroy: true
});
}
});

0 comments on commit f3b0cf5

Please sign in to comment.