diff --git a/build.js b/build.js index 3910fe7..2c6b212 100755 --- a/build.js +++ b/build.js @@ -95,7 +95,8 @@ function afterBuild() { var installPath = path.join(__dirname, 'bin', modPath, 'fibers.node'); try { - fs.mkdirSync(path.join(__dirname, 'bin', modPath)); + // recursive: a source-only package (no prebuilt binaries) has no bin/ directory at all + fs.mkdirSync(path.join(__dirname, 'bin', modPath), { recursive: true }); } catch (ex) {} try { diff --git a/fibers_async.js b/fibers_async.js index 9546a9d..966ea58 100644 --- a/fibers_async.js +++ b/fibers_async.js @@ -11,7 +11,8 @@ function Fiber(fn, ...args) { const ar = new AsyncResource('Fiber'); const actualFn = (...args1) => ar.runInAsyncScope(() => { Fiber.current._meteor_dynamics = undefined; - fn(...args1); + // return the fiber function's value so run() resolves to it when the fiber finishes (fibers README semantics) + return fn(...args1); }); const _fiber = _Fiber(actualFn, ...args); asyncResourceWeakMap.set(_fiber, ar); @@ -27,6 +28,19 @@ Object.defineProperty(Fiber, 'current', { } }) +// poolSize is a native data property on the native constructor. On V8 >= 13 (node 24) assigning +// `Fiber.poolSize = n` on this wrapper no longer reaches that setter: it creates an own property on +// the wrapper and the native pool size silently stays at its default (120). Forward it explicitly, +// which is also what happened implicitly on node 18. +Object.defineProperty(Fiber, 'poolSize', { + get() { + return _Fiber.poolSize; + }, + set(value) { + _Fiber.poolSize = value; + }, +}) + _Fiber.prototype.runInAsyncScope = function runInAsyncScope(fn) { return asyncResourceWeakMap.get(this).runInAsyncScope(fn); diff --git a/src/coroutine.cc b/src/coroutine.cc index e89abdc..a592561 100644 --- a/src/coroutine.cc +++ b/src/coroutine.cc @@ -210,13 +210,16 @@ Coroutine::Coroutine(entry_t& entry, void* arg) : } Coroutine::~Coroutine() { - if (stack.sptr) { - coro_stack_free(&stack); - } + // Destroy the context before freeing the stack: with CORO_PTHREAD the coroutine is an OS thread + // whose struct pthread glibc places at the top of the user-supplied stack, so coro_destroy's + // pthread_cancel/pthread_join must run while that memory is still mapped. #ifdef CORO_FIBER if (context.fiber) #endif (void)coro_destroy(&context); + if (stack.sptr) { + coro_stack_free(&stack); + } } Coroutine* Coroutine::create_fiber(entry_t* entry, void* arg) { diff --git a/src/fibers.cc b/src/fibers.cc index 68311a1..cf86a51 100644 --- a/src/fibers.cc +++ b/src/fibers.cc @@ -202,11 +202,13 @@ namespace uni { args.GetReturnValue().Set(handle); } template - void Return(Local handle, GetterCallbackInfo info) { + // PropertyCallbackInfo must be taken by reference: since V8 13 it holds the argument slots inline, + // so GetReturnValue().Set() on a by-value copy is silently lost. + void Return(Local handle, const GetterCallbackInfo& info) { info.GetReturnValue().Set(handle); } template - void Return(Persistent& handle, GetterCallbackInfo info) { + void Return(Persistent& handle, const GetterCallbackInfo& info) { info.GetReturnValue().Set(Local::New(Isolate::GetCurrent(), handle)); } @@ -357,7 +359,23 @@ namespace uni { } #endif -#if V8_AT_LEAST(6, 1) +#if V8_AT_LEAST(13, 0) + // FunctionCallbackInfo::Holder() was removed in V8 13; This() is the documented replacement. + inline Local Holder(const FunctionCallbackInfo& args) { return args.This(); } +#else + inline Local Holder(const FunctionCallbackInfo& args) { return args.Holder(); } +#endif + +#if V8_AT_LEAST(13, 0) + // ObjectTemplate::SetAccessor / Object::SetAccessor were removed in V8 13; SetNativeDataProperty is the replacement. + void SetAccessor( + Isolate* isolate, Local object, Local name, + FunctionType (*getter)(Local, const GetterCallbackInfo&), + void (*setter)(Local property, Local value, const SetterCallbackInfo&) = 0 + ) { + object->SetNativeDataProperty(isolate->GetCurrentContext(), name, (AccessorNameGetterCallback)getter, (AccessorNameSetterCallback)setter).ToChecked(); + } +#elif V8_AT_LEAST(6, 1) void SetAccessor( Isolate* isolate, Local object, Local name, FunctionType (*getter)(Local, const GetterCallbackInfo&), @@ -462,6 +480,7 @@ class Fiber { * i.e. After fiber completes, while yielded, or before started */ void MakeWeak() { + if (handle.IsEmpty()) return; // garbage-collected fiber being unwound as a zombie uni::MakeWeak(isolate, handle, (void*)this); } @@ -470,6 +489,7 @@ class Fiber { * i.e. While running. */ void ClearWeak() { + if (handle.IsEmpty()) return; // see MakeWeak() handle.ClearWeak(); } @@ -490,7 +510,14 @@ class Fiber { if (that.started) { assert(that.yielding); orphaned_fibers.push_back(&that); +#if V8_AT_LEAST(10, 4) + // kParameter is a phantom callback: V8 has already reclaimed the JS object and CHECKs that + // the handle was reset before this callback returns ("Handle not reset in first callback"). + // The fiber is unwound and deleted by DestroyOrphans; there is no JS object to hand back. + uni::Dispose(that.isolate, that.handle); +#else that.ClearWeak(); +#endif return; } @@ -532,7 +559,12 @@ class Fiber { } uni::Dispose(that.isolate, that.yielded); +#if V8_AT_LEAST(10, 4) + // The JS object is gone (see WeakCallback); nothing can reference this fiber again. + delete &that; +#else that.MakeWeak(); +#endif } } @@ -560,7 +592,7 @@ class Fiber { * be created and the callback will start. Otherwise we switch back into the exist context. */ static uni::FunctionType Run(const uni::Arguments& args) { - Fiber& that = Unwrap(args.Holder()); + Fiber& that = Unwrap(uni::Holder(args)); // There seems to be no better place to put this check.. DestroyOrphans(); @@ -601,7 +633,7 @@ class Fiber { * Throw an exception into a currently yielding fiber. */ static uni::FunctionType ThrowInto(const uni::Arguments& args) { - Fiber& that = Unwrap(args.Holder()); + Fiber& that = Unwrap(uni::Holder(args)); if (!that.yielding) { THROW(Exception::Error, "This Fiber is not yielding"); @@ -622,7 +654,7 @@ class Fiber { * effect. */ static uni::FunctionType Reset(const uni::Arguments& args) { - Fiber& that = Unwrap(args.Holder()); + Fiber& that = Unwrap(uni::Holder(args)); if (!that.started) { return uni::Return(uni::Undefined(that.isolate), args); @@ -833,7 +865,8 @@ class Fiber { } static uni::FunctionType GetCurrent(Local property, const uni::GetterCallbackInfo& info) { - if (current) { + if (current && !current->handle.IsEmpty()) { + // The handle is empty while a garbage-collected fiber is being unwound as a zombie. return uni::Return(current->handle, info); } else { return uni::Return(uni::Undefined(Isolate::GetCurrent()), info); @@ -891,7 +924,11 @@ class Fiber { uni::NewFunctionTemplate(isolate, Run, Local(), sig)); proto->Set(uni::NewLatin1Symbol(isolate, "throwInto"), uni::NewFunctionTemplate(isolate, ThrowInto, Local(), sig)); +#if V8_AT_LEAST(13, 0) + proto->SetNativeDataProperty(uni::NewLatin1Symbol(isolate, "started"), (AccessorNameGetterCallback)GetStarted); +#else proto->SetAccessor(uni::NewLatin1Symbol(isolate, "started"), GetStarted); +#endif // Global yield() function Local yield = uni::GetFunction(uni::NewFunctionTemplate(isolate, Yield_)); diff --git a/test/orphan-gc.js b/test/orphan-gc.js new file mode 100644 index 0000000..eacdb07 --- /dev/null +++ b/test/orphan-gc.js @@ -0,0 +1,32 @@ +"use strict"; +// A yielded fiber whose JS object is garbage-collected must be unwound as a zombie, not crash the +// process. On V8 >= 10.4 the weak callback is a phantom (kParameter) callback and has to reset its +// handle; the old kFinalizer code path resurrected the object instead. +var Fiber = require('fibers'); +var v8 = require('v8'); +var vm = require('vm'); +v8.setFlagsFromString('--expose_gc'); +var gc = vm.runInNewContext('gc'); + +var N = 200, unwound = 0; +for (var ii = 0; ii < N; ++ii) { + var fiber = Fiber(function() { + try { + Fiber.yield(); + } catch (err) { + // zombie exception. Touch Fiber.current: on V8 >= 10.4 the JS object is already gone and + // this must return undefined instead of dereferencing an empty handle. + Fiber.current; + ++unwound; + throw err; + } + }); + fiber.run(); + fiber = null; +} +gc(); gc(); +Fiber(function() {}).run(); // Fiber::Run() calls DestroyOrphans() +gc(); gc(); +Fiber(function() {}).run(); + +console.log(unwound === N ? 'pass' : 'fail: unwound ' + unwound + '/' + N);