Skip to content

Commit

Permalink
src: make AsyncResource destructor virtual
Browse files Browse the repository at this point in the history
`AsyncResource` is intended to be a base class, and since we don’t
know what API consumers will do with it in their own code,
it’s good practice to make its destructor virtual.

This should not be ABI-breaking since all class methods are inline.

PR-URL: #20633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax authored and rvagg committed Aug 16, 2018
1 parent 87818dc commit 342dbff
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,7 @@ class AsyncResource {
trigger_async_id);
}

~AsyncResource() {
virtual ~AsyncResource() {
EmitAsyncDestroy(isolate_, async_context_);
}

Expand Down
22 changes: 22 additions & 0 deletions test/addons/async-resource/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ using v8::Object;
using v8::String;
using v8::Value;

int custom_async_resource_destructor_calls = 0;

class CustomAsyncResource : public AsyncResource {
public:
CustomAsyncResource(Isolate* isolate, Local<Object> resource)
: AsyncResource(isolate, resource, "CustomAsyncResource") {}
~CustomAsyncResource() {
custom_async_resource_destructor_calls++;
}
};

void CreateAsyncResource(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
assert(args[0]->IsObject());
Expand Down Expand Up @@ -98,6 +109,16 @@ void GetResource(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(r->get_resource());
}

void RunSubclassTest(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
Local<Object> obj = Object::New(isolate);

assert(custom_async_resource_destructor_calls == 0);
CustomAsyncResource* resource = new CustomAsyncResource(isolate, obj);
delete static_cast<AsyncResource*>(resource);
assert(custom_async_resource_destructor_calls == 1);
}

void Initialize(Local<Object> exports) {
NODE_SET_METHOD(exports, "createAsyncResource", CreateAsyncResource);
NODE_SET_METHOD(exports, "destroyAsyncResource", DestroyAsyncResource);
Expand All @@ -107,6 +128,7 @@ void Initialize(Local<Object> exports) {
NODE_SET_METHOD(exports, "getAsyncId", GetAsyncId);
NODE_SET_METHOD(exports, "getTriggerAsyncId", GetTriggerAsyncId);
NODE_SET_METHOD(exports, "getResource", GetResource);
NODE_SET_METHOD(exports, "runSubclassTest", RunSubclassTest);
}

} // anonymous namespace
Expand Down
2 changes: 2 additions & 0 deletions test/addons/async-resource/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const assert = require('assert');
const binding = require(`./build/${common.buildType}/binding`);
const async_hooks = require('async_hooks');

binding.runSubclassTest();

const kObjectTag = Symbol('kObjectTag');
const rootAsyncId = async_hooks.executionAsyncId();

Expand Down

0 comments on commit 342dbff

Please sign in to comment.