Node-addon-api: `Error: Invalid argument` when calling Napi::Function

[devsnek]

I'm working on a native EventEmitter, and i'm getting invalid argument when calling the handlers in a loop, but nowhere else. see the comments in the screenshots for more info.

[bnoordhuis]

Can you post a more complete code example? It's impossible to tell what the issue is right now.

[devsnek]

https://github.com/devsnek/eventemitter-native/blob/169e53ad0fad96179aeb797572d7cdb92761b947/src/EventEmitter.cc#L93-L100

[bnoordhuis]

Don't know if it's the cause but by the looks of it, the once and every collections can get mutated while they're being iterated over (loop calls into JS, JS can call mutator methods.)

[devsnek]

@bnoordhuis so is this a napi issue or should i close this

[bnoordhuis]

I'd close it unless you've conclusively established it's a napi issue.

[devsnek]

@bnoordhuis i guess this is out of scope of the issue, but do you have any knowledge of how to fix this? i'm not very experienced with any of the addon apis

[bnoordhuis]

Iterate over a copy of the list. The built-in EventEmitter does the same thing.

[devsnek]

@bnoordhuis i tried making a copy but it has the same error, i'm hopeful i just did this completely wrong

    std::vector<Napi::Function> copy(this->every[name].begin(), this->every[name].end());
    for (Napi::Function const& handler: copy)
      handler.Call(callArgs); // same error as before

[devsnek]

after talking with my friend who actually knows c++ i'm gonna repoen this

[bnoordhuis]

UB how and where?

[guymguym]

@devsnek I might be off here, but seems to me like you keep the functions in your class members after the javascript scope that contained them (the scope of the On() function) already returned, and the local reference you had is invalidated.

In order to keep values after the local scope returns you will need to create a reference, which for convinience has an Napi::FunctionReference class you can use:
https://nodejs.github.io/node-addon-api/class_napi_1_1_function_reference.html

Notice that even the Napi::String values you used for keys are invalidated so you better just use std::string for your map keys.

You should still make a copy of the vector before calling the handlers in case the handlers will decide to call On() or RemoveListener() which will change the vector during the iteration, but then you can just take out the value from the ref - see below.

This code seems to do what you want.

addon.cpp:

#include <iostream>
#include <vector>
#include <map>
#include <napi.h>

class Emitter : public Napi::ObjectWrap<Emitter>
{
private:
    typedef std::vector<Napi::FunctionReference> Handlers;
    typedef std::map<std::string, Handlers> HandlersMap;
    HandlersMap handlers_map;

public:
    static void
    Init(Napi::Env env, Napi::Object exports)
    {
        exports["Emitter"] = DefineClass(
            env,
            "Emitter",
            {
                InstanceMethod("on", &Emitter::on),
                InstanceMethod("emit", &Emitter::emit),
            }
        );
    }

    Emitter(const Napi::CallbackInfo &info) : Napi::ObjectWrap<Emitter>(info) {}

    void
    on(const Napi::CallbackInfo &info)
    {
        Napi::String event = info[0].As<Napi::String>();
        Napi::Function func = info[1].As<Napi::Function>();
        handlers_map[event].push_back(
            Napi::Reference<Napi::Function>::New(func, 1)
        );
    }

    void
    emit(const Napi::CallbackInfo &info)
    {
        Napi::String event = info[0].As<Napi::String>();
        std::vector<Napi::Function> list;
        for (Napi::FunctionReference &ref : handlers_map[event]) {
            list.push_back(ref.Value());
        }
        for (Napi::Function &func : list) {
            func.Call({});
        }
    }
};

Napi::Object
addon(Napi::Env env, Napi::Object exports)
{
    Emitter::Init(env, exports);
    return exports;
}

NODE_API_MODULE(addon, addon)

test.js:

'use strict';

const { Emitter } = require('./build/Release/addon');

const e = new Emitter();
e.on('lala', () => console.log('LALA1'));
e.on('lala', () => console.log('LALA2'));
e.on('lala', () => console.log('LALA3'));
e.emit('lala');

Output:

$ node test.js 
LALA1
LALA2
LALA3
(node:13429) Warning: N-API is an experimental feature and could change at any time.

[guymguym]

@bnoordhuis I think this issue can be closed. Hope that helps.

[mhdawson]

@guymguym thanks for providing a detailed answer.

[vinnitu]

how to pass arguments to Call() properly?