Repository navigation
Segfault when using node-addon-api Objects during AsyncWorker::Execute #281
Description
In the following project:
https://github.com/bitflip-software/xlsx-util/tree/feature/async-worker
branch feature/async-worker
f3b31420281e5f5939ad082c6835f024c14cd728
When I run the project's test with npm run test, I get a segfault.
I've been chasing this down for days, and today I built Node from source (v9.8) so that I could debug.
It appears that all of the objects my code is operating on are being destroyed when uv__fs_done is called, which calls the suspicious looking node::InternalCallbackScope::Close()
But my code is still running on another thread, which ends in tears.
My AsyncWorker looks like this
#include "AsyncReader.h"
#include "XlsxReaderFunctions.h"
namespace xlsx
{
AsyncReader::AsyncReader( Napi::HandleScope scope, const std::string& filename, bool hasHeaders, const Napi::Function& headerTransform, const Napi::Function& callback )
: Napi::AsyncWorker{ callback }
, myScope{ scope }
, myFilename{ filename }
, myHasHeaders{ hasHeaders }
, myHeaderTransform{ headerTransform }
, myResult{}
{
}
void
AsyncReader::Execute()
{
myResult = extractAllData( myScope, myFilename, myHasHeaders, myHeaderTransform );
}
void
AsyncReader::OnOK()
{
Callback().MakeCallback( Receiver().Value(), { myScope.Env().Null(), myResult } );
}
void
AsyncReader::OnError( const Napi::Error& e )
{
Callback().MakeCallback( Receiver().Value(), { e.Value(), myScope.Env().Undefined() } );
}
}
The AsyncReader is instantiated and queued like this:
#include "napi.h"
#include "readXlsxAsync.h"
#include "AsyncReader.h"
#include "AsyncError.h"
namespace xlsx
{
void readXlsxAsync( const Napi::CallbackInfo& info )
{
Napi::Env env = info.Env();
Napi::HandleScope scope{ env };
// we need to check for the presence of a client callback function
// and if we do not have one then we must raise a synchronous error
if( info.Length() != 4 )
{
Napi::TypeError::New(env, "invalid argument count").ThrowAsJavaScriptException();
return;
}
if( !info[3].IsFunction() )
{
Napi::TypeError::New(env, "invalid callback function [arg3]").ThrowAsJavaScriptException();
return;
}
Napi::Function cb = info[3].As<Napi::Function>();
if( !info[0].IsString() )
{
auto err = new AsyncError{ cb, "first argument must be a string [filename]" };
err->Queue();
return;
}
if( !info[1].IsBoolean() )
{
auto err = new AsyncError{ cb, "second argument must be a bool [hasHeaders]" };
err->Queue();
return;
}
if( !info[2].IsFunction() )
{
auto err = new AsyncError{ cb, "third argument must be a function [headerTransform]" };
err->Queue();
return;
}
const std::string filename = info[0].As<Napi::String>().Utf8Value();
const bool hasHeaders = info[1].As<Napi::Boolean>();
Napi::Function headerTransform = info[2].As<Napi::Function>();
AsyncReader* reader = new AsyncReader{ scope, filename, hasHeaders, headerTransform, cb };
reader->Queue();
}
}
xlsx::extractAllData runs synchronously on a single thread (as far as I understand, though it does create Napi::Objects.
Bottom line, I can't understand why my code segfaults because the lifetime management of these libraries is very complicated and I'm new to this (and Napi:: doesn't seem to have much documentation).
When does an AsyncWorker destroy itself exactly? Can anyone tell me why my code segfaults?
Activity
mhdawson commentedon Jun 13, 2018
Which version of Node.js are you using? Can you also add the full strack trace you get when it crashes ?
mhdawson commentedon Jun 13, 2018
Also, do you run napi, or JS code from Execute() ? Our doc was not clear enough on the C side, PR to clarify is here: nodejs/node#21217. I'd have to double check but I expect the same will apply in the Execute() method for the async worker.
webern commentedon Jun 13, 2018
The production Node build is 9.8.0 but I also segfault in the version of Node that I build with Xcode for debugging, which is 9.11.3.
The napi version is
The backtrace is:
webern commentedon Jun 13, 2018
I am running plain native code and napi.h code from my Execute() function. I am not reaching into the N-API C api myself.
mhdawson commentedon Jun 13, 2018
Ok, executing napi.h code from Execute() can cause JavaScript to execute which is not ok off the main thread. You need to limit what is in Execute() to plain native code and put anything that needs to interact with JavaScript in the OnOK() method.
mhdawson commentedon Jun 13, 2018
This is a pull request with work-in-progress documentation: #272
webern commentedon Jun 13, 2018
OK, that's sounds like a good next attempt. It makes it hard to do my use case efficiently, but oh well.
webern commentedon Jun 13, 2018
One more important question before I re-write. Does
OnOKblock the JavaScript event loop? That would be bad for my use case because I need to marshal a significant amount of data from the native code into JavaScript objects. Thanks again.mhdawson commentedon Jun 14, 2018
Everything on the main thread will block the event loop, and OnOK runs on that loop.
Are you moving an existing module over to N-API or is this a new module? I don't think this is any different from modules written with nan, but asking to be sure.
Is it possible to get the results incrementally? ie have the native code do whatever processing is necessary and return a native structure. Then incrementally extract the data through multiple calls to the addon from JavaScript so that it does not block for the full time?
[-]Exactly When does AsyncWorker destroy itself?[/-][+]Segfault when using node-addon-api Objects during AsyncWorker::Execute[/+]webern commentedon Jun 14, 2018
I am new to JavaScript as well as Node.js addons. This is my first attempt at a Node.js addon. I started using
v8.handnode.hand can't remember how I chose to switch tonapi.h. I haven't use NAN so haven't tried to compare anything to NAN.In general, if it is impossible to access the 'JavaScript' objects on a separate thread, then Node.js itself looks like a bad choice for my project since marshaling my data back-and-forth from native objects to JavaScript objects might negate the performance gains of doing operations in native code. Ultimately it feels like I should have chosen Go for this project.
Anyway, in my use-case, blocking the main thread for 10 seconds or so isn't the end of the world, I'm just disappointed by Node.js limitations for my use case. I need to learn about worker farms or child processes next to see if that can help. What's not clear to me yet is whether or not I can pass a huge array of JavaScript objects to a child process or worker and receive those objects back.
I will open a new thread for additional issues that I am having with AsyncWorker and close this issue as you have resolved the segfault problem I was having.
Also note that I added a comment in the new AsyncWorker documentation pull request #272 asking for more detail about how the object is destroyed.
Thanks.
mhdawson commentedon Jun 14, 2018
@webern thanks for the comments and input.