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

napi_threadsafe_function -- Segmentation fault: 11 #1387

Closed
jazz-soft opened this issue Jul 16, 2018 · 6 comments
Closed

napi_threadsafe_function -- Segmentation fault: 11 #1387

jazz-soft opened this issue Jul 16, 2018 · 6 comments

Comments

@jazz-soft
Copy link

  • Node.js Version: 10.6.0
  • OS: MacOS 10.13.6
  • Scope (install, code, runtime, meta, other?): N-API
  • Module (and version) (if relevant): node-gyp

Getting segmentation fault on exit in MacOS and Linux, no error output in Windows.
Have I missed something in the c++ code?
Sorry, the documentation at https://nodejs.org/api/n-api.html#n_api_asynchronous_thread_safe_function_calls was a bit unclear...

Thank you!

#define NAPI_EXPERIMENTAL
#include <node_api.h>
#include <iostream>

namespace test {

napi_threadsafe_function TSF;

napi_value my_thread(napi_env env, napi_callback_info args)
{
    std::cout << "my_thread...\n";

    napi_value value;
    napi_get_undefined(env, &value);
    return value;
}

void my_finalize(napi_env env, void* finalize_data, void* finalize_hint)
{
    std::cout << "my_finalize...\n";
}

void my_callback(napi_env env, napi_value js_callback, void* context, void* data)
{
    std::cout << "my_callback...\n";
}

napi_value start(napi_env env, napi_callback_info args)
{
    std::cout << "start...\n";

    napi_value func;
    napi_value str;
    napi_create_function(env, 0, 0, my_thread, 0, &func);
    napi_create_string_utf8(env, "no_name", NAPI_AUTO_LENGTH, &str);
    napi_create_threadsafe_function(env, func, 0, str, 0, 1, 0, my_finalize, 0, my_callback, &TSF);

    napi_value value;
    napi_get_undefined(env, &value);
    return value;
}

napi_value stop(napi_env env, napi_callback_info args)
{
    std::cout << "stop...\n";

    napi_release_threadsafe_function(TSF, napi_tsfn_release);
    napi_unref_threadsafe_function(env, TSF);

    napi_value value;
    napi_get_undefined(env, &value);
    return value;
}

napi_value init(napi_env env, napi_value exports)
{
    napi_value value;

    // incorrect function signature at https://nodejs.org/api/n-api.html#n_api_napi_create_function_1
    napi_create_function(env, "start", NAPI_AUTO_LENGTH, start, 0, &value);
    napi_set_named_property(env, exports, "start", value);

    // incorrect function signature at https://nodejs.org/api/n-api.html#n_api_napi_create_function_1
    napi_create_function(env, "stop", NAPI_AUTO_LENGTH, stop, 0, &value);
    napi_set_named_property(env, exports, "stop", value);

    return exports;
}

NAPI_MODULE(NODE_GYP_MODULE_NAME, init)

}  // namespace test
var test = require('.');
test.start();
test.stop();
setTimeout(function() {
  console.log('done!');
}, 500);
@mhdawson
Copy link
Member

@gabrielschulhof can you take a look at this?

@gabrielschulhof
Copy link

@jazz-soft the thread-safe function is meant to be used with an existing threads implementation. Creating a thread-safe function will not cause N-API to spawn new threads.

The semantics of napi_create_threadsafe_function() are basically this: "Turn this napi_value which refers to a JavaScript function into such a JavaScript function that I can call from another thread – a napi_threadsafe_function." How you start another thread, and how you pass the napi_threadsafe_function to that other thread is not part of the API.

It is very important that napi_release_threadsafe_function() be the last time you pass the thread-safe function to any of the APIs because passing it to napi_release_threadsafe_function() might result in its deallocation. That is, in the above code, you should not pass the thread-safe function to napi_unref_threadsafe_function() after having first passed it to napi_release_threadsafe_function().

Since napi_threadsafe_function is a recent addition to N-API, it's documentation is also a first pass. Thus, could you please be more specific as to in what way the documentation "was a bit unclear?" That should help me improve it.

Overall, from the usage pattern in the code sample you provide, it looks to me like you might be better served by napi_create_async_work() and its related APIs, because that set of APIs does cause a function you provide to be executed on another thread.

@jazz-soft
Copy link
Author

@gabrielschulhof thank you for explanation.
After removing napi_unref_threadsafe_function() as you advised, I'm still getting the segmentation fault.
Is there anything else missing?

napi_value stop(napi_env env, napi_callback_info args)
{
    std::cout << "stop...\n";

    napi_release_threadsafe_function(TSF, napi_tsfn_release); // also tried with napi_tsfn_abort 

    napi_value value;
    napi_get_undefined(env, &value);
    return value;
}

As for documentation, it would be great to have a complete copy-pasteable threadsafe_function lifecycle example similar to the one in my original post.

napi_create_async_work() does not fit my needs. I do have another thread that wants to call JavaScript, but I have removed it to have the minimal example of the code that still fails.

@gabrielschulhof
Copy link

@jazz-soft it turns out that there were no tests for the case where queue size is zero, meaning unlimited. In those conditions, napi_call_threadsafe_function() never blocks, nor does it return napi_queue_full, and, crucially, no condition variable is created internally, meaning it's set to null. Since no test was creating a thread-safe function with an unlimited queue size, the null condition variable was being dereferenced sometimes.

Good catch!

@jazz-soft
Copy link
Author

I'm glad my complain was helpful :)
Please let me know in which release the fix will be present.
And thanks a lot for fixing it!

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Jul 19, 2018
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
targos pushed a commit to nodejs/node that referenced this issue Jul 24, 2018
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: #21871
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@ralphtheninja
Copy link

I've also noticed this. Was browsing through the napi code and noticed that if you set the queue size to something, then it didn't crash. Happy someone else found it and that it got fixed. I'm guessing for now, a workaround is to actually set queue size to something > 0.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Dec 28, 2018
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: nodejs#21871
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jan 18, 2019
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: #21871
Backport-PR-URL: #25002
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
rvagg pushed a commit to nodejs/node that referenced this issue Feb 28, 2019
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: #21871
Backport-PR-URL: #25002
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.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