Skip to content

Commit 7679068

Browse files
author
Gabriel Schulhof
committed
src: add AsyncWorker destruction suppression
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an instance of the class to remain allocated even after the `OnOK` callback fires. Such an instance must be explicitly `delete`-ed from user code. Re: #231 Re: nodejs/abi-stable-node#353
1 parent d8e9c22 commit 7679068

File tree

8 files changed

+122
-2
lines changed

8 files changed

+122
-2
lines changed

doc/async_worker.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,15 @@ the computation that happened in the `Napi::AsyncWorker::Execute` method, unless
6666
the default implementation of `Napi::AsyncWorker::OnOK` or
6767
`Napi::AsyncWorker::OnError` is overridden.
6868

69+
### SuppressDestruct
70+
71+
```cpp
72+
void Napi::AsyncWorker::SuppressDestruct();
73+
```
74+
75+
Prevents the destruction of the `Napi::AsyncWorker` instance upon completion of
76+
the `Napi::AsyncWorker::OnOK` callback.
77+
6978
### SetError
7079

7180
Sets the error message for the error that happened during the execution. Setting

napi-inl.h

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3524,7 +3524,8 @@ inline AsyncWorker::AsyncWorker(const Object& receiver,
35243524
const Object& resource)
35253525
: _env(callback.Env()),
35263526
_receiver(Napi::Persistent(receiver)),
3527-
_callback(Napi::Persistent(callback)) {
3527+
_callback(Napi::Persistent(callback)),
3528+
_suppress_destruct(false) {
35283529
napi_value resource_id;
35293530
napi_status status = napi_create_string_latin1(
35303531
_env, resource_name, NAPI_AUTO_LENGTH, &resource_id);
@@ -3550,6 +3551,7 @@ inline AsyncWorker::AsyncWorker(AsyncWorker&& other) {
35503551
_receiver = std::move(other._receiver);
35513552
_callback = std::move(other._callback);
35523553
_error = std::move(other._error);
3554+
_suppress_destruct = other._suppress_destruct;
35533555
}
35543556

35553557
inline AsyncWorker& AsyncWorker::operator =(AsyncWorker&& other) {
@@ -3560,6 +3562,7 @@ inline AsyncWorker& AsyncWorker::operator =(AsyncWorker&& other) {
35603562
_receiver = std::move(other._receiver);
35613563
_callback = std::move(other._callback);
35623564
_error = std::move(other._error);
3565+
_suppress_destruct = other._suppress_destruct;
35633566
return *this;
35643567
}
35653568

@@ -3589,6 +3592,10 @@ inline FunctionReference& AsyncWorker::Callback() {
35893592
return _callback;
35903593
}
35913594

3595+
inline void AsyncWorker::SuppressDestruct() {
3596+
_suppress_destruct = true;
3597+
}
3598+
35923599
inline void AsyncWorker::OnOK() {
35933600
_callback.Call(_receiver.Value(), std::initializer_list<napi_value>{});
35943601
}
@@ -3629,7 +3636,9 @@ inline void AsyncWorker::OnWorkComplete(
36293636
return nullptr;
36303637
});
36313638
}
3632-
delete self;
3639+
if (!self->_suppress_destruct) {
3640+
delete self;
3641+
}
36333642
}
36343643

36353644
////////////////////////////////////////////////////////////////////////////////

napi.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1722,6 +1722,7 @@ namespace Napi {
17221722

17231723
void Queue();
17241724
void Cancel();
1725+
void SuppressDestruct();
17251726

17261727
ObjectReference& Receiver();
17271728
FunctionReference& Callback();
@@ -1760,6 +1761,7 @@ namespace Napi {
17601761
ObjectReference _receiver;
17611762
FunctionReference _callback;
17621763
std::string _error;
1764+
bool _suppress_destruct;
17631765
};
17641766

17651767
// Memory management.

test/asyncworker-persistent.cc

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
#include "napi.h"
2+
3+
// A variant of TestWorker wherein destruction is suppressed. That is, instances
4+
// are not destroyed during the `OnOK` callback. They must be explicitly
5+
// destroyed.
6+
7+
using namespace Napi;
8+
9+
namespace {
10+
11+
class PersistentTestWorker : public AsyncWorker {
12+
public:
13+
static PersistentTestWorker* current_worker;
14+
static void DoWork(const CallbackInfo& info) {
15+
bool succeed = info[0].As<Boolean>();
16+
Function cb = info[1].As<Function>();
17+
18+
PersistentTestWorker* worker = new PersistentTestWorker(cb, "TestResource");
19+
current_worker = worker;
20+
21+
worker->SuppressDestruct();
22+
worker->_succeed = succeed;
23+
worker->Queue();
24+
}
25+
26+
static Value WorkerGone(const CallbackInfo& info) {
27+
return Boolean::New(info.Env(), current_worker == nullptr);
28+
}
29+
30+
static void DeleteWorker(const CallbackInfo& info) {
31+
(void) info;
32+
delete current_worker;
33+
}
34+
35+
~PersistentTestWorker() {
36+
current_worker = nullptr;
37+
}
38+
39+
protected:
40+
void Execute() override {
41+
if (!_succeed) {
42+
SetError("test error");
43+
}
44+
}
45+
46+
private:
47+
PersistentTestWorker(Function cb,
48+
const char* resource_name)
49+
: AsyncWorker(cb, resource_name) {}
50+
51+
bool _succeed;
52+
};
53+
54+
PersistentTestWorker* PersistentTestWorker::current_worker = nullptr;
55+
56+
} // end of anonymous namespace
57+
58+
Object InitPersistentAsyncWorker(Env env) {
59+
Object exports = Object::New(env);
60+
exports["doWork"] = Function::New(env, PersistentTestWorker::DoWork);
61+
exports["workerGone"] = Function::New(env, PersistentTestWorker::WorkerGone);
62+
exports["deleteWorker"] =
63+
Function::New(env, PersistentTestWorker::DeleteWorker);
64+
return exports;
65+
}

test/asyncworker-persistent.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
'use strict';
2+
const buildType = process.config.target_defaults.default_configuration;
3+
const assert = require('assert');
4+
const common = require('./common');
5+
const binding = require(`./build/${buildType}/binding.node`);
6+
const noexceptBinding = require(`./build/${buildType}/binding_noexcept.node`);
7+
8+
function test(binding, succeed) {
9+
return new Promise((resolve) =>
10+
// Can't pass an arrow function to doWork because that results in an
11+
// undefined context inside its body when the function gets called.
12+
binding.doWork(succeed, function() {
13+
setImmediate(() => {
14+
assert.strictEqual(binding.workerGone(this), false);
15+
binding.deleteWorker(this);
16+
assert.strictEqual(binding.workerGone(this), true);
17+
resolve();
18+
});
19+
}));
20+
}
21+
22+
test(binding.persistentasyncworker, false, 'binding')
23+
.then(() => test(binding.persistentasyncworker, true, 'binding'))
24+
.then(() => test(noexceptBinding.persistentasyncworker, false, 'noxbinding'))
25+
.then(() => test(noexceptBinding.persistentasyncworker, true, 'noxbinding'));

test/binding.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ using namespace Napi;
66
Object InitArrayBuffer(Env env);
77
Object InitAsyncContext(Env env);
88
Object InitAsyncWorker(Env env);
9+
Object InitPersistentAsyncWorker(Env env);
910
Object InitBasicTypesArray(Env env);
1011
Object InitBasicTypesBoolean(Env env);
1112
Object InitBasicTypesNumber(Env env);
@@ -42,6 +43,7 @@ Object Init(Env env, Object exports) {
4243
exports.Set("arraybuffer", InitArrayBuffer(env));
4344
exports.Set("asynccontext", InitAsyncContext(env));
4445
exports.Set("asyncworker", InitAsyncWorker(env));
46+
exports.Set("persistentasyncworker", InitPersistentAsyncWorker(env));
4547
exports.Set("basic_types_array", InitBasicTypesArray(env));
4648
exports.Set("basic_types_boolean", InitBasicTypesBoolean(env));
4749
exports.Set("basic_types_number", InitBasicTypesNumber(env));

test/binding.gyp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
'arraybuffer.cc',
99
'asynccontext.cc',
1010
'asyncworker.cc',
11+
'asyncworker-persistent.cc',
1112
'basic_types/array.cc',
1213
'basic_types/boolean.cc',
1314
'basic_types/number.cc',
@@ -43,6 +44,12 @@
4344
'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED']
4445
}, {
4546
'sources': ['object/object_deprecated.cc']
47+
}],
48+
['OS!="windows"', {
49+
'cflags+': ['-fvisibility=hidden'],
50+
'xcode_settings': {
51+
'OTHER_CFLAGS': ['-fvisibility=hidden']
52+
}
4653
}]
4754
],
4855
'include_dirs': ["<!@(node -p \"require('../').include\")"],

test/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ let testModules = [
1111
'arraybuffer',
1212
'asynccontext',
1313
'asyncworker',
14+
'asyncworker-persistent',
1415
'basic_types/array',
1516
'basic_types/boolean',
1617
'basic_types/number',

0 commit comments

Comments
 (0)