FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

async_wrap: call destroy() callback in uv_idle_t · nodejs/node@f3b0cf5 · GitHub

/ node Public

Commit f3b0cf5

Browse files
trevnorris
authored and
MylesBorins
committed
async_wrap: call destroy() callback in uv_idle_t
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>
1 parent 3e5b2eb commit f3b0cf5

7 files changed

Lines changed: 69 additions & 25 deletions

File tree

‎src/async-wrap.cc‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "util.h"
66
#include "util-inl.h"
77

8+
#include "uv.h"
89
#include "v8.h"
910
#include "v8-profiler.h"
1011

@@ -182,6 +183,38 @@ void AsyncWrap::Initialize(Local<Object> target,
182183
}
183184

184185

186+
void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) {
187+
uv_idle_stop(handle);
188+
189+
Environment* env = Environment::from_destroy_ids_idle_handle(handle);
190+
// None of the V8 calls done outside the HandleScope leak a handle. If this
191+
// changes in the future then the SealHandleScope wrapping the uv_run()
192+
// will catch this can cause the process to abort.
193+
HandleScope handle_scope(env->isolate());
194+
Context::Scope context_scope(env->context());
195+
Local<Function> fn = env->async_hooks_destroy_function();
196+
197+
if (fn.IsEmpty())
198+
return env->destroy_ids_list()->clear();
199+
200+
TryCatch try_catch(env->isolate());
201+
202+
for (auto current_id : *env->destroy_ids_list()) {
203+
// Want each callback to be cleaned up after itself, instead of cleaning
204+
// them all up after the while() loop completes.
205+
HandleScope scope(env->isolate());
206+
Local<Value> argv = Number::New(env->isolate(), current_id);
207+
MaybeLocal<Value> ret = fn->Call(
208+
env->context(), Undefined(env->isolate()), 1, &argv);
209+
210+
if (ret.IsEmpty()) {
211+
ClearFatalExceptionHandlers(env);
212+
FatalException(env->isolate(), try_catch);
213+
}
214+
}
215+
}
216+
217+
185218
void LoadAsyncWrapperInfo(Environment* env) {
186219
HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler();
187220
#define V(PROVIDER) \
@@ -248,18 +281,10 @@ AsyncWrap::~AsyncWrap() {
248281
if (!ran_init_callback())
249282
return;
250283

251-
Local<Function> fn = env()->async_hooks_destroy_function();
252-
if (!fn.IsEmpty()) {
253-
HandleScope scope(env()->isolate());
254-
Local<Value> uid = Number::New(env()->isolate(), get_uid());
255-
TryCatch try_catch(env()->isolate());
256-
MaybeLocal<Value> ret =
257-
fn->Call(env()->context(), Null(env()->isolate()), 1, &uid);
258-
if (ret.IsEmpty()) {
259-
ClearFatalExceptionHandlers(env());
260-
FatalException(env()->isolate(), try_catch);
261-
}
262-
}
284+
if (env()->destroy_ids_list()->empty())
285+
uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb);
286+
287+
env()->destroy_ids_list()->push_back(get_uid());
263288
}
264289

265290

‎src/async-wrap.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
55

66
#include "base-object.h"
7+
#include "uv.h"
78
#include "v8.h"
89

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

64+
static void DestroyIdsCb(uv_idle_t* handle);
65+
6366
inline ProviderType provider_type() const;
6467

6568
inline int64_t get_uid() const;

‎src/env-inl.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,8 @@ inline Environment::Environment(v8::Local<v8::Context> context,
245245

246246
RB_INIT(&cares_task_list_);
247247
handle_cleanup_waiting_ = 0;
248+
249+
destroy_ids_list_.reserve(512);
248250
}
249251

250252
inline Environment::~Environment() {
@@ -305,6 +307,15 @@ inline uv_idle_t* Environment::immediate_idle_handle() {
305307
return &immediate_idle_handle_;
306308
}
307309

310+
inline Environment* Environment::from_destroy_ids_idle_handle(
311+
uv_idle_t* handle) {
312+
return ContainerOf(&Environment::destroy_ids_idle_handle_, handle);
313+
}
314+
315+
inline uv_idle_t* Environment::destroy_ids_idle_handle() {
316+
return &destroy_ids_idle_handle_;
317+
}
318+
308319
inline Environment* Environment::from_idle_prepare_handle(
309320
uv_prepare_t* handle) {
310321
return ContainerOf(&Environment::idle_prepare_handle_, handle);
@@ -381,6 +392,10 @@ inline int64_t Environment::get_async_wrap_uid() {
381392
return ++async_wrap_uid_;
382393
}
383394

395+
inline std::vector<int64_t>* Environment::destroy_ids_list() {
396+
return &destroy_ids_list_;
397+
}
398+
384399
inline uint32_t* Environment::heap_statistics_buffer() const {
385400
CHECK_NE(heap_statistics_buffer_, nullptr);
386401
return heap_statistics_buffer_;

‎src/env.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "v8.h"
1717

1818
#include <stdint.h>
19+
#include <vector>
1920

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

415416
static inline Environment* from_immediate_check_handle(uv_check_t* handle);
417+
static inline Environment* from_destroy_ids_idle_handle(uv_idle_t* handle);
416418
inline uv_check_t* immediate_check_handle();
417419
inline uv_idle_t* immediate_idle_handle();
420+
inline uv_idle_t* destroy_ids_idle_handle();
418421

419422
static inline Environment* from_idle_prepare_handle(uv_prepare_t* handle);
420423
inline uv_prepare_t* idle_prepare_handle();
@@ -451,6 +454,9 @@ class Environment {
451454

452455
inline int64_t get_async_wrap_uid();
453456

457+
// List of id's that have been destroyed and need the destroy() cb called.
458+
inline std::vector<int64_t>* destroy_ids_list();
459+
454460
inline uint32_t* heap_statistics_buffer() const;
455461
inline void set_heap_statistics_buffer(uint32_t* pointer);
456462

@@ -543,6 +549,7 @@ class Environment {
543549
IsolateData* const isolate_data_;
544550
uv_check_t immediate_check_handle_;
545551
uv_idle_t immediate_idle_handle_;
552+
uv_idle_t destroy_ids_idle_handle_;
546553
uv_prepare_t idle_prepare_handle_;
547554
uv_check_t idle_check_handle_;
548555
AsyncHooks async_hooks_;
@@ -558,6 +565,7 @@ class Environment {
558565
bool trace_sync_io_;
559566
size_t makecallback_cntr_;
560567
int64_t async_wrap_uid_;
568+
std::vector<int64_t> destroy_ids_list_;
561569
debugger::Agent debugger_agent_;
562570
#if HAVE_INSPECTOR
563571
inspector::Agent inspector_agent_;

‎src/node.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4511,6 +4511,9 @@ Environment* CreateEnvironment(Isolate* isolate,
45114511
uv_unref(reinterpret_cast<uv_handle_t*>(env->idle_prepare_handle()));
45124512
uv_unref(reinterpret_cast<uv_handle_t*>(env->idle_check_handle()));
45134513

4514+
uv_idle_init(env->event_loop(), env->destroy_ids_idle_handle());
4515+
uv_unref(reinterpret_cast<uv_handle_t*>(env->destroy_ids_idle_handle()));
4516+
45144517
// Register handle cleanups
45154518
env->RegisterHandleCleanup(
45164519
reinterpret_cast<uv_handle_t*>(env->immediate_check_handle()),

‎test/parallel/test-async-wrap-throw-from-callback.js‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ const assert = require('assert');
66
const crypto = require('crypto');
77
const domain = require('domain');
88
const spawn = require('child_process').spawn;
9-
const callbacks = [ 'init', 'pre', 'post', 'destroy' ];
9+
const callbacks = [ 'init', 'pre', 'post' ];
1010
const toCall = process.argv[2];
1111
var msgCalled = 0;
1212
var msgReceived = 0;
@@ -23,13 +23,9 @@ function post() {
2323
if (toCall === 'post')
2424
throw new Error('post');
2525
}
26-
function destroy() {
27-
if (toCall === 'destroy')
28-
throw new Error('destroy');
29-
}
3026

3127
if (typeof process.argv[2] === 'string') {
32-
async_wrap.setupHooks({ init, pre, post, destroy });
28+
async_wrap.setupHooks({ init, pre, post });
3329
async_wrap.enable();
3430

3531
process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE'));

‎test/parallel/test-async-wrap-uid.js‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,14 @@ const assert = require('assert');
66
const async_wrap = process.binding('async_wrap');
77

88
const storage = new Map();
9-
async_wrap.setupHooks({ init, pre, post, destroy });
9+
async_wrap.setupHooks({ init, pre, post });
1010
async_wrap.enable();
1111

1212
function init(uid) {
1313
storage.set(uid, {
1414
init: true,
1515
pre: false,
1616
post: false,
17-
destroy: false
1817
});
1918
}
2019

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

29-
function destroy(uid) {
30-
storage.get(uid).destroy = true;
31-
}
32-
3328
fs.access(__filename, function(err) {
3429
assert.ifError(err);
3530
});
@@ -51,7 +46,6 @@ process.once('exit', function() {
5146
init: true,
5247
pre: true,
5348
post: true,
54-
destroy: true
5549
});
5650
}
5751
});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL