Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to get reference counter value from Napi::Reference object #702

Closed
ikokostya opened this issue Apr 11, 2020 · 14 comments
Closed

Ability to get reference counter value from Napi::Reference object #702

ikokostya opened this issue Apr 11, 2020 · 14 comments

Comments

@ikokostya
Copy link
Contributor

There is no way to get reference count value from Napi::Reference object without side effects. I can use only Ref() or Unref() method, which returns the resulting reference count value.

What is the reason to not export this functionality as public API?

@mhdawson
Copy link
Member

There was not specific reason, other than we did not think of it as requirement. What is your use case for it?

@ikokostya
Copy link
Contributor Author

ikokostya commented Apr 13, 2020

What is your use case for it?

I want to use ref counter for detecting if there are no active workers for some object.

Consider some class with asynchronous methods:

class Foo {
  foo(): Promise<void>;
  bar(): Promise<void>;

  dispose(): Promise<void>;
}

Each async method increases ref counter for Foo instance to prevent GC. In this case native object can be disposed only if ref count is equal to 0:

class Foo : public Napi::ObjectWrap<Foo> { 
  // ...
};

class DisposeWorker : public Napi::AsyncWorker {
  // ...
};

Napi::Value Foo::Dispose(const Napi::CallbackInfo& info) {
  if (RefCount() > 0) {
    // Execute worker in future when ref count will be equal to 0.
    pending_dispose_worker_ = new DisposeWorker();
    return;
  }
  // Call worker immediately.
  auto worker = new DisposeWorker();
  worker->Queue();
}

@gabrielschulhof
Copy link
Contributor

@ikokostya if you want to execute some code when a certain Napi::Object goes out of scope you can use Napi::Object::AddFinalizer to attach a callback that will be called when the Napi::Object is about to be garbage-collected.

@gabrielschulhof
Copy link
Contributor

@ikokostya something like this might work:

class Foo: public Napi::ObjectWrap<Foo> {
 public:
  Foo(const Napi::FunctionCallbackInfo& info) {
    auto worker = new DisposeWorker();
    info
      .This()
      .As<Napi::Object>()
      .AddFinalizer([](Napi::Env env, DisposeWorker* worker) {
        worker->Queue();
      }, worker);
  }
};

@ikokostya
Copy link
Contributor Author

ikokostya commented Apr 20, 2020

@gabrielschulhof I have another use case. The dispose worker calls non-trivial destructor on the corresponding native object:

class DisposeWorker : public AsyncWorker {
 public:
  DisposeWorker(Napi::Env env, Foo* foo) : AsyncWorker(env), foo_(foo) {
    foo_->Ref();
  }

  ~DisposeWorker() {
    foo_->Unref();
  }

 protected:
  void Execute() override {
    foo_->native_handle.release(); // where native_handle is std::unique_ptr<NativeFoo>
  }

 private:
  Foo* foo_;
}; 

When dispose will be called from JavaScript

await foo.dispose();

I cannot just queue new worker

auto worker = new DisposeWorker(info.Env(), this);
worker->Queue();

because there may exist another worker threads on the native object. Instead, I need to wait while other worker threads will be executed before start the dispose worker.

Napi::Object::AddFinalizer doesn't help me here, because JavaScript code keeps reference to the foo object and GC will not be called.

@davedoesdev
Copy link
Contributor

Should finalizer always be called before napi_get_reference_value starts returning null for result?

I'm seeing occasions where my finalizer gets called later, meaning Reference#Value() is throwing an Invalid Argument exception.

@mhdawson
Copy link
Member

mhdawson commented May 8, 2020

I don't believe there is any guarantee as to when the finalizer will be called except that it will be sometime after the object is collected. This means it is reasonable for napi_get_reference_value to return NULL before the finalizer is run.

Do you have a small recreate? The code in Value checks if the
_ref is nullptr so I'm wondering how the call to napi_get_reference_value is being passed an invalid value.

@davedoesdev
Copy link
Contributor

I can try to get a small recreate. But in the meantime I believe the problem to be that napi_get_reference_value in Value() is succeeding but returning NULL in value.
The invalid argument exception then comes from the call to T(_env, value):
In my case T is Buffer<uint8_t> so it ends up calling TypedArrayOf<T>::TypedArrayOf(napi_env env, napi_value value). This then calls napi_get_typedarray_info which fails and results in an error being called.

@davedoesdev
Copy link
Contributor

davedoesdev commented May 12, 2020

@mhdawson I got a small repro.

stalereference.cc

#include "napi.h"

using namespace Napi;

static Reference<Buffer<uint8_t>> weak;

void CreateWeak(const CallbackInfo& info) {
    weak = Weak(Buffer<uint8_t>::New(info.Env(), 1));
    weak.SuppressDestruct();
}

void CallValue(const CallbackInfo&) {
    if (!weak.IsEmpty()) {
        weak.Value();
    }
}

Object InitStaleReference(Env env) {
    Object exports = Object::New(env);

    exports["createWeak"] = Function::New(env, CreateWeak);
    exports["callValue"] = Function::New(env, CallValue);

    return exports;
}

stalereference.js

'use strict';
const buildType = process.config.target_defaults.default_configuration;

const test = binding => {
    binding.stalereference.createWeak();
    global.gc();
    binding.stalereference.callValue();
};

test(require(`./build/${buildType}/binding.node`));
test(require(`./build/${buildType}/binding_noexcept.node`));

@mhdawson
Copy link
Member

Thanks, I've created a test we can add to our test suite and am looking at how we should fix.

mhdawson added a commit to mhdawson/node-addon-api that referenced this issue May 13, 2020
Fixes: nodejs#702

Previously calling Value() on a Reference for a TypedArray
that the enderlying object had been collected would result
in an error due to a failure in creating the return value.

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member

@davedoesdev can you check out the fix in #726.

Note that you will need to call IsEmpty() on the value after getting it back from weak.Value() to check if the value was returned or not. The weak.IsEmpty() does not check if the underlying object is present or not just if the weak itself wraps a N-API reference.

@davedoesdev
Copy link
Contributor

Yep, works for me - thanks!

@ikokostya
Copy link
Contributor Author

I solved my original problem without Napi::Reference. Just to be clear, current value of Napi::Reference instance can be got using napi_get_reference_value function. Closing issue.

@ikokostya
Copy link
Contributor Author

Oops, missed that there is open associated PR

@ikokostya ikokostya reopened this May 14, 2020
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
Fixes: nodejs/node-addon-api#702

Previously calling Value() on a Reference for a TypedArray
that the enderlying object had been collected would result
in an error due to a failure in creating the return value.

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs/node-addon-api#726
Fixes: nodejs/node-addon-api#702
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
Fixes: nodejs/node-addon-api#702

Previously calling Value() on a Reference for a TypedArray
that the enderlying object had been collected would result
in an error due to a failure in creating the return value.

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs/node-addon-api#726
Fixes: nodejs/node-addon-api#702
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
Fixes: nodejs/node-addon-api#702

Previously calling Value() on a Reference for a TypedArray
that the enderlying object had been collected would result
in an error due to a failure in creating the return value.

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs/node-addon-api#726
Fixes: nodejs/node-addon-api#702
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
austinli64 added a commit to austinli64/node-addon-api that referenced this issue May 9, 2023
Fixes: nodejs/node-addon-api#702

Previously calling Value() on a Reference for a TypedArray
that the enderlying object had been collected would result
in an error due to a failure in creating the return value.

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs/node-addon-api#726
Fixes: nodejs/node-addon-api#702
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
Fixes: nodejs/node-addon-api#702

Previously calling Value() on a Reference for a TypedArray
that the enderlying object had been collected would result
in an error due to a failure in creating the return value.

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs/node-addon-api#726
Fixes: nodejs/node-addon-api#702
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants