Nan::Callback not suited for subclassing. Why? And why is it not final then?

[nineninesevenfour]

[label:question] [label:newbie]

While going through the "OFFLOADING THE WORK" exercise in the goingnative workshopper, I wanted to know if this statement about implicit clean-up is really true:

To use MyWorker and Nan::Callback you need to allocate memory on the "heap" for them by using the new operator. NAN will perform clean-up of both objects for you so you don't need a matching delete in this case as you normally would in C++.

So I basically did:

class MyCallback : public Nan::Callback {
public:
    MyCallback(Local<Function> cbFunction) : Nan::Callback(cbFunction) {
        cout << "constructor of MyCallback" << endl;
    }
    ~MyCallback() {
        cout << "destructor of MyCallback" << endl;
    }
};

..and..

Nan::Callback *callback = new MyCallback(cbFunction);

Unfortunately "destructor of MyCallback" was never printed to the console. :cry: After poking around in nan.h I found the reason in the missing virtual keyword of Nan::Callback: :nerd_face:

  /* no virtual */ ~Callback() {
    handle_.Reset();
  }

So my questions are:

• Is there a reason for ommiitting the virtual keyword at ~Callback()?
• If so, wouldn't it be better to make Nan::Callback final to prevent such mistakes?

[addaleax]

I think the answer here is that a) nobody thought that somebody would want to subclass Nan::Callback and b) Nan predates the mandatory use of C++11 for Node.js addons, so final just wasn’t available when this was first written. I think adding final here is fine at this point (with #if __cplusplus >= 201103L).

[nineninesevenfour]

@addaleax Thank you Anna, for pointing that out! 👍 I have to admit that my CPP knowledge is quite rusted and spoiled by so many years of Java development. It felt for me like the final keyword has been always around. 🙄 After having a second look it seems that mostly the AsyncWorker classes are meant to be subclassed, while the rest is rather not. I wouldn't rather introduce a new pattern for just one class. Therefor I simply close this issue and next time I'll have a look closer on the things I try to use. 😉

[agnat]

@nineninesevenfour, thank you for pointing it out.

@addaleax Yep, looks pretty final to me... Unsure about closing it in a minor release, though. Others might have subclassed the thing not noticing the broken behaviour and we might break downstream builds. Although, now that I think about it the risk is probably minute...