workshopper / goingnative

A NodeSchool style workshopper for learning how to write native Node.js addons
MIT License
415 stars 54 forks source link

Running "Offloading The Work" fails w/ anonymous function #38

Open johnnyman727 opened 10 years ago

johnnyman727 commented 10 years ago

I'm working on the final tutorial, "Offloading The Work".

My solution fails if my delay is invoked as follows with provided code:

addon.delay(process.argv[2], function() {
  clearInterval(interval)
  console.log('Done!')
})

By making the callback non-anonymous, the solution passes:

addon.delay(process.argv[2], function callback() {
  clearInterval(interval)
  console.log('Done!')
})
johnnyman727 commented 10 years ago

Actually, I lied. It seems to be a race condition (surprise)!

It will inconsistently pass or fail regardless of whether the callback is named.

rvagg commented 10 years ago

ok, so can you paste your native addon code here and are you using NanCallback to wrap your Function object when passing it to async?

johnnyman727 commented 10 years ago

Hi @rvagg,

Yes, I'm wrapping my callback in a NanCallback to keep it from being gc'ed.

Here is a copy of my addon.cc

#include <nan.h>

// at the top of your file
#ifndef _WIN32
#include <unistd.h>
#endif

using namespace v8;
class MyWorker : public NanAsyncWorker {
 public:
  // Constructor
  MyWorker(NanCallback *callback, int delay)
    : NanAsyncWorker(callback), delay(delay) {}
  // Destructor
  ~MyWorker() {}

  // Executed inside the worker-thread.
  // It is not safe to access V8, or V8 data structures
  // here, so everything we need for input and output
  // should go on `this`.
  void Execute () {
    // Asynchronous, non-V8 work goes here
    #ifdef _WIN32
    Sleep(delay);
    #else
    usleep(delay * 1000);
    #endif
  }

  // Executed when the async work is complete
  // this function will be run inside the main event loop
  // so it is safe to use V8 again
  void HandleOKCallback () {
    NanScope();

    // NanCallback#Call() does a NanMakeCallback() for us
    callback->Call(0, NULL);
  }

 private:
  int delay;
};

NAN_METHOD(Delay) {
  NanScope();

  // get delay and callback
  Local<Number> num = args[0].As<Number>();
  Local<Function> callback = args[1].As<Function>();

  // create NanCallback instance wrapping the callback
  NanCallback *protectedCallback = new NanCallback(callback);

  // create a MyWorker instance, passing the callback and delay
  // MyWorker
  MyWorker *worker = new MyWorker(protectedCallback, num->IntegerValue());

  // queue the worker instance onto the thread-pool
  NanAsyncQueueWorker(worker);

  NanReturnUndefined();
}

NAN_METHOD(Sleep) {

  Local<Number> num = args[0].As<Number>();
  Local<Function> callback = args[1].As<Function>();

  #ifdef _WIN32
  Sleep(num->IntegerValue());
  #else
  usleep(num->IntegerValue() * 1000);
  #endif

  NanMakeCallback(NanGetCurrentContext()->Global(), callback, 0, NULL);

  NanReturnUndefined();
}

NAN_METHOD(Length) {
  NanScope();
  Local<String> str = args[0].As<String>();
  int len = strlen(*String::Utf8Value(str));
  Local<Number> num = NanNew<Number>(len);
  NanReturnValue(num);
}

NAN_METHOD(Print) {
  Local<String> str = args[0].As<String>();
  printf("%s\n", *String::Utf8Value(str));
  NanReturnUndefined();
}

void Init(Handle<Object> exports) {
  exports->Set(NanNew("print"), NanNew<FunctionTemplate>(Print)->GetFunction());
  exports->Set(NanNew("length"), NanNew<FunctionTemplate>(Length)->GetFunction());
  exports->Set(NanNew("sleep"), NanNew<FunctionTemplate>(Sleep)->GetFunction());
  exports->Set(NanNew("delay"), NanNew<FunctionTemplate>(Delay)->GetFunction());

}

NODE_MODULE(myaddon, Init);
kkoopa commented 10 years ago

Strange, I don't see any errors, apart from NODE_MODULE(myaddon, Init); having a semicolon, it should not have that -- but I don't think this is impacting the runtime behavior.

johnnyman727 commented 10 years ago

@kkoopa, are you able to repro?

rvagg commented 10 years ago

@johnnyman727 what version of Node is this? node -v

johnnyman727 commented 10 years ago

v0.10.26

rvagg commented 10 years ago

@johnnyman727 try this and see if it changes anything:

addon.delay(process.argv[2], function() {
  clearInterval(interval)
  console.log('Done!')
  addon.foo = 'bar'
})
johnnyman727 commented 10 years ago

@rvagg unfortunately, it still passes inconsistently.

rvagg commented 10 years ago

@johnnyman727 as in, it sometimes passes and sometimes fails?

johnnyman727 commented 10 years ago

Yeah, it will fail somewhere around 9 times out of 10.

dsewtz commented 9 years ago

Similar problem here, running under MSYS:

exercise.js(89) expect = /FAUX 1\nFAUX 2\nWaiting._FAUX 3\n.._FAUX 4\nDone!\n/m My output (and that of the original solution) is: FAUX 1\nFAUX 2\nFAUX 3\nWaiting..FAUX 4\nDone!

Giving the NanReturn a little more time to resume javascript execution, fixed this for me: goingnative\exercises\offloading_the_work\faux\myaddon.cc(21) : Sleep(100);

I also had to go full kirk on the kobayashi maru/"AmIReady?"-test by adding: exercise.js(163): , 2010: 'VS100COMNTOOLS'

Thanks for the great tutorial! This way I also learned a lot about the global npm- and node-module directory ;-)