Support Node 0.11.4 #184

Merged
merged 10 commits into from Jun 20, 2014

Projects

None yet

9 participants

Contributor
kkoopa commented Aug 13, 2013

This is a backwards-compatible patch for Node 0.11.4+. It addresses #180 #151 #177. Travis is still stuck at 11.0.3, so it does not compile there.

Member

Hi @kkoopa - thanks for the pull. Looks like you've put a lot of effort into the nan project. As I mention in #180, I plan to take a review the v8/node c++ changes once node v0.12.x is out. At that time I will review your pull and decide whether nan is appropriate. In the meantime, a few thoughts:

  1. What about having projects that use nan.h depend on it as an actual node module via package.json. Then nan releases could be pushed to npmjs. For this to work the binding.gyp for a given project would simply need to put -I./node_modules/nan/ in the include flags.

  2. Do you have any benchmarks of whether nan.h usage helps or hurts performance with node < v0.11.4?

Contributor
kkoopa commented Aug 13, 2013

Yes,the idea is to have it as a module configurable exactly like that. This will come in the near future, at latest when 0.12 arrives.

I have not benchmarked this, as I haven't tried it on other platforms than Linux, and I only have a VM available, which does not seem to offer a realistic benchmarking experience.

I did however do some benchmarking of hiredis-node redis/hiredis-node#40 (comment) and it showed no performance regressions. And from theory I can state that performance on old versions will be equal or better, as all of NAN is a set of conditional macros and inline functions.

Contributor
kkoopa commented Aug 13, 2013

As mentioned in kkoopa#1 it does not work on 0.11.0 < Node < 0.11.4, as they are outdated development releases and there is no point in supporting them. It works on 0.11.4+. Travis is still stuck at 0.11.3.

@Mithgol Mithgol referenced this pull request in kkoopa/node-sqlite3 Aug 14, 2013
Merged

undo my previous commit #2

Contributor
kkoopa commented Aug 19, 2013

NAN is available through NPM now.

Member

@kkoopa - I'm planning on taking a closer look at this patch in the coming weeks. Any updates you see as needed based on more movement in node core or NAN?

Contributor
kkoopa commented Oct 29, 2013

Updated NAN dependency to 0.4.1, which is the latest. Node 0.11.8 or should arrive at some point, but this builds with latest HEAD. There have been some deprecations in v8, so expect a new minor NAN release for 0.11.8.

rvagg commented Dec 6, 2013

yes please! I'd like to start being able to benchmark on 0.11.

Contributor
kkaefer commented Dec 6, 2013

@rvagg Could you please explain what the benefit of making nan a dependency of this module would be? I'm not seeing large amounts of dropped code; mostly just wrapping existing callbacks.

Contributor
kkoopa commented Dec 6, 2013

You're looking at it from the wrong side, it's about not having large amounts of duplicated code.

Contributor
kkaefer commented Dec 6, 2013

Sure, but this module wasn't using nan before, and your pull request adds it. Yet, there's no removed code?

Contributor
kkoopa commented Dec 6, 2013

As explained in the commit message and #180, #151 and #177, the current version of node-sqlite3 does not work and will not work on anything newer than Node.js v0.10.x. That currently excludes the 0.11-series, which will become the 0.12 series in a couple of months. When 0.12 is released, all further development of 0.10 will stop. The reason it will not work is because a significant part of the API in v8 has been changed in an incompatible way.

As some people like to use outdated software, which might be legitimate due to stability concerns, this causes a problem for native addon developers, as their codebase suddenly has to support two very different platforms. The three basic options are then: support the outdated platform and lose all users with current versions, support the new platform and ignore the old production users, or make and maintain two separate versions for each group.

The best solution is actually none of these, it is wrapping all incompatible functions in an abstraction layer, unifying the two disjoint architectures and maintaining one version of the code, supporting both. This pull request offers supporting both architectures without code duplication.

rvagg commented Dec 6, 2013

To see why NAN is useful, consider these two snippets that do exactly the same thing, i.e. declare a method to expose to JS, but one is for Node 0.10 and prior and the other is for 0.11 and onwards:

Node 0.10 and prior:

v8::Handle<v8::Value> DoSomething (const v8::Arguments& args) {
  v8::HandleScope scope;

  // ...

  return scope.Close(v8::String::New("We did something"));
}

Node 0.11+

void DoSomething (const v8::FunctionCallbackInfo<v8::Value>& args) {
  v8::Isolate* isolate = v8::Isolate::GetCurrent();
  v8::HandleScope scope(isolate);

  // ...

  args.GetReturnValue().Set(v8::String::New("We did something"));
}

It would be difficult to be more different! The V8 team decided to do some major API reworking and that's been bleeding into Node since around 0.11.3. NAN is all about making it unnecessary to have macro branching in your addon code just so you can support the different versions of Node that you want. NAN tests from 0.8 up to Node master and we're always adding new stuff as more V8 breakages come in. There's also some Node breakages that we smooth over too, currently this is mainly for Buffers. We also apparently support Node 0.6 but don't bother testing there.

The idea is that you include nan.h and code against the NAN API which provides a very thin layer (mostly macros) between your code and Node + V8 but it takes care of compatibility for you and will continue to take care of it as V8 and Node evolve. The alternative for Node 0.11/0.12 is going to be to drop support for 0.10 and prior or have the code full of #ifs and #elses for every one of these changes, and there are a lot of them.

The NAN version of the above code is this:

NAN_METHOD(DoSomething) {
  NanScope();

  // ...

  NanReturnValue(v8::String::New("We did something"));
}

This expands out to be whatever the target version of Node/V8 is wherever it's being compiled.

rvagg commented Dec 6, 2013

Also, more directly answering the question about why pull it in as a dependency: the initial versions of NAN just distributed a header file that you put in your project and distribute yourself but now we're distributing it via npm so you don't need to have your own copy, you just pull it in via npm and compile against it. If you pin the dependency as ~0.6.0 then you would get any patch-level releases too which would include minor fixes to support current Node master as things change. It's basically developing against C++ assets like you do against JS assets in npm so you can think of it in the same way.

I believe the initial version of this pull request had nan.h embedded but that's gone now in favour of having it as a dependency.

nan is a very good thing if your goal is to support node 0.10 and 0.12 (when it lands). Without NaN you would either be in #ifdef hell or be implementing all of nan yourself.

Contributor
kkaefer commented Dec 9, 2013

Okay, thanks @rvagg for explaining the rationale behind this pull request. This all makes sense.

Contributor
jeromew commented Jan 5, 2014

thank you @rvagg for your clear explanation of nan and how node 0.11.x is a potential nightmare for the "native modules" ecosystem in npm.

@springmeyer can you share your opinion on this and do you have an idea where this could fit in the node-sqlite3 roadmap ?

I am investigating the use of the new koa.js - http://koajs.com/ framework written by the main contributors of express.js but it only works with node 0.11.9 at this stage so I have to either go without koa.js or without node-sqlite3 which is tearing me apart ;-)

note that I only use "build-from-source" with node-sqlite3 so in my case I don't need a precompiled node-sqlite3 working with node 0.11.x but only the patched source code using Nan.

as a side node, nan was apparently accepted on node-postgres (commit by @rvagg ) here - brianc/node-postgres@f58ff73

Contributor
jeromew commented Jan 6, 2014

For information i tried the following today (I am using sqlite 3.8.1):

git clone --depth=1 https://github.com/kkoopa/node-sqlite3.git
cd node-sqlite3
nvm use 0.11.9
npm install --build-from-source --sqlite=/path/to/custom/sqlite

and get :

In file included from /home/user1/.node-gyp/0.11.9/src/node.h:61:0,
             from ../src/database.cc:2:
/home/user1/.node-gyp/0.11.9/deps/v8/include/v8.h: In member function ‘void v8::ReturnValue<T>::Set(uint32_t)’:
/home/user1/.node-gyp/0.11.9/deps/v8/include/v8.h:5816:31: warning: typedef ‘I’ locally defined but not used [-    Wunused-local-typedefs]
typedef internal::Internals I;
... several times and then
SOLINK_MODULE(target) Release/obj.target/node_sqlite3.node: Finished
...
[sqlite3]: Testing the binary failed: "Command failed: /home/user1/nvm/v0.11.9/bin/node: symbol lookup error: /home/user1/tmp/node-sqlite3/lib/binding/Release/node-v13-linux-x64/node_sqlite3.node: undefined symbol: _ZN2v87Integer3NewEi

The same steps work perfectly with

nvm use 0.10.22
...
[sqlite3]: Sweet: "node_sqlite3.node" is valid, node-sqlite3 is now installed!

I looked around a bit but have a hard time understanding what goes wrong. @koopa what are your steps to compile node-sqlite3 with 0.11.9 ?

Thank you

Contributor
kkoopa commented Jan 6, 2014

My initial guess is that your custom sqlite3 build is linking with gcc rather than g++.

Contributor
jeromew commented Jan 6, 2014

Looking into this (the hard way I am not a link expert ;-)

regarding the warning on "typedef internal::Internals I;", I don't think it is part of the problem.

This warning is pretty explicit in the v8.h code for this version of node

template<typename T>
void ReturnValue<T>::Set(uint32_t i) {
  TYPE_CHECK(T, Integer);
  typedef internal::Internals I;                             <==== NOT USED
  // Can't simply use INT32_MAX here for whatever reason.
  bool fits_into_int32_t = (i & (1U << 31)) == 0;
  if (V8_LIKELY(fits_into_int32_t)) {
    Set(static_cast<int32_t>(i));
    return;
  }
  Set(Integer::NewFromUnsigned(i, GetIsolate()));
}

The typedef has been removed from v8 trunk : https://code.google.com/p/v8/source/browse/trunk/include/v8.h#5789

template<typename T>
void ReturnValue<T>::Set(uint32_t i) {
  TYPE_CHECK(T, Integer);
                                                                       <==== removed
  // Can't simply use INT32_MAX here for whatever reason.
  bool fits_into_int32_t = (i & (1U << 31)) == 0;
  if (V8_LIKELY(fits_into_int32_t)) {
    Set(static_cast<int32_t>(i));
    return;
  }
  Set(Integer::NewFromUnsigned(GetIsolate(), i));
}

now back to the missing symbol. it seems to me that g++ is used by node-sqlite3 at link time ; can it really depend on my sqlite3 custom build (a pretty standard sqlite3 build configure/make/make install) ?

Member

@jeromew - thanks for helping test and push this forward.

@springmeyer can you share your opinion on this and do you have an idea where this could fit in the node-sqlite3 roadmap?

I plan to 1) upgrade the bundled libsqlite3 version, 2) merge this, and 3) migrate the binary build system to node-pre-gyp when I find the time. I'm fully on board with using NAN and I am very thankful for the work put in to make it available. I anticipate I will be able to do this sometime in the next couple weeks.

Contributor
jeromew commented Jan 7, 2014

Hello. Here are the results of my research on this. Maybe @kkoopa or @rvagg you can take a look at these results. There is something weird somewhere that I don't understand.

Long story short :

1.the missing symbol is _ZN2v87Integer3NewEi
2.it can indeed be found in the compiled native extension :

 $ nm -A lib/binding/Release/node-v13-linux-x64/node_sqlite3.node | grep '_ZN2v87Integer3NewEi'                                           
 lib/binding/Release/node-v13-linux-x64/node_sqlite3.node:                 U _ZN2v87Integer3NewEi

3.but cannot be found in node v 0.11.9

$ nm -A /home/u/nvm/v0.11.9/bin/node | grep '_ZN2v87Integer3NewEi'
/home/u/nvm/v0.11.9/bin/node:0000000000703670 T _ZN2v87Integer3NewEiPNS_7IsolateE

4.instead we find this symbol _ZN2v87Integer3NewEiPNS_7IsolateE

5.different hints lead me to this block of code in v8.h

class V8_EXPORT Integer : public Number {
 public:
  static Local<Integer> New(int32_t value);
  static Local<Integer> NewFromUnsigned(uint32_t value);
  static Local<Integer> New(int32_t value, Isolate*);
  static Local<Integer> NewFromUnsigned(uint32_t value, Isolate*);
  int64_t Value() const;
  V8_INLINE static Integer* Cast(v8::Value* obj);
 private:
   Integer();
  static void CheckCast(v8::Value* obj);
};

6.it appears that only an "Isolated" version of New(uint32_t) exists in nvm node 0.11.9

7.I could not find any Nan helper that handles this case

8.note that the signature (int32_t, Isolate_) is reversed from NanNewLocal (Isolate_, Local)

9.the compilation works if I add to Nan something like

 static NAN_INLINE(v8::Local<v8::Integer> NanNewInteger(
      int32_t val
  )) {
    return v8::Integer::New(val, nan_isolate);
  }

10.and modify the node-sqlite3 code with

 cd src
 sed -i'' -e's/Integer::New/NanNewInteger/g' *

11.result: no more missing symbol at built time

 [sqlite3]: Sweet: "node_sqlite3.node" is valid, node-sqlite3 is now installed!

12.but this is not the end of the story

npm test
Creating test database... This may take several minutes.
node: symbol lookup error: /home/u/tmp/node_modules/sqlite3/lib/binding/Release/node-v13-linux-x64/node_sqlite3.node: undefined symbol: _ZNK2v85Value8IsRegExpEv

13.this probably belong to source->IsRegExp() and args[start]->IsRegExp() of statement.cc

14.this time less luck with nm

nm -A /home//nvm/v0.11.9/bin/node | grep -e 'Value[0-9]*Is'
/home//nvm/v0.11.9/bin/node:0000000000ee9458 b _ZGVZNK2v85Value7IsInt32EvE10minus_zero
/home//nvm/v0.11.9/bin/node:0000000000ee9448 b _ZGVZNK2v85Value8IsUint32EvE10minus_zero
/home//nvm/v0.11.9/bin/node:00000000008f6550 T _ZN2v88internal16CompileTimeValue18IsCompileTimeValueEPNS0_10ExpressionE
/home//nvm/v0.11.9/bin/node:00000000007c8e50 T _ZN2v88internal6HValue19IsInteger32ConstantEv
/home//nvm/v0.11.9/bin/node:0000000000729af0 W _ZN2v88internal6HValue29IsPurelyInformativeDefinitionEv
/home//nvm/v0.11.9/bin/node:00000000006fa760 T _ZNK2v85Value10IsExternalEv
/home//nvm/v0.11.9/bin/node:00000000006fa670 T _ZNK2v85Value10IsFunctionEv
/home//nvm/v0.11.9/bin/node:00000000006fd7a0 T _ZNK2v85Value6IsTrueEv
/home//nvm/v0.11.9/bin/node:00000000006fa6a0 T _ZNK2v85Value7IsArrayEv
/home//nvm/v0.11.9/bin/node:00000000006fd770 T _ZNK2v85Value7IsFalseEv
/home//nvm/v0.11.9/bin/node:00000000006fbe90 T _ZNK2v85Value7IsInt32Ev
/home//nvm/v0.11.9/bin/node:00000000006fa700 T _ZNK2v85Value8IsNumberEv
/home//nvm/v0.11.9/bin/node:00000000006fa6d0 T _ZNK2v85Value8IsObjectEv
/home//nvm/v0.11.9/bin/node:00000000006fbd70 T _ZNK2v85Value8IsUint32Ev
/home//nvm/v0.11.9/bin/node:00000000006fa730 T _ZNK2v85Value9IsBooleanEv
/home//nvm/v0.11.9/bin/node:00000000007c3220 W _ZNK2v88internal6HValue10IsConstantEv
/home//nvm/v0.11.9/bin/node:0000000000729ba0 W _ZNK2v88internal6HValue11IsDeletableEv
/home//nvm/v0.11.9/bin/node:00000000007c3240 W _ZNK2v88internal6HValue13IsInstructionEv
/home//nvm/v0.11.9/bin/node:00000000007c5110 T _ZNK2v88internal6HValue14IsDefinedAfterEPNS0_11HBasicBlockE
/home//nvm/v0.11.9/bin/node:00000000007299d0 W _ZNK2v88internal6HValue17IsBinaryOperationEv
/home//nvm/v0.11.9/bin/node:00000000007299f0 W _ZNK2v88internal6HValue20IsControlInstructionEv
/home//nvm/v0.11.9/bin/node:00000000007299e0 W _ZNK2v88internal6HValue24IsBitwiseBinaryOperationEv
/home//nvm/v0.11.9/bin/node:00000000007299c0 W _ZNK2v88internal6HValue27IsArithmeticBinaryOperationEv
/home//nvm/v0.11.9/bin/node:0000000000ee9460 b _ZZNK2v85Value7IsInt32EvE10minus_zero
/home//nvm/v0.11.9/bin/node:0000000000ee9450 b _ZZNK2v85Value8IsUint32EvE10minus_zero

15.and

nm -C /home/jwagner/nvm/v0.11.9/bin/node | grep -e 'IsRegExp'
00000000009db7d0 T v8::internal::FullCodeGenerator::EmitIsRegExp(v8::internal::CallRuntime*)
00000000009d73d0 T v8::internal::FullCodeGenerator::EmitIsRegExpEquivalent(v8::internal::CallRuntime*)
00000000007ec360 T v8::internal::HOptimizedGraphBuilder::GenerateIsRegExp(v8::internal::CallRuntime*)
00000000007d9d90 T v8::internal::HOptimizedGraphBuilder::GenerateIsRegExpEquivalent(v8::internal::CallRuntime*)

So IsRegExp does not seem to be exported.

I have learned a lot of things doing this but I am a bit stalled understanding why those symbols are not in the binaries given by "nvm install 0.11.9" (http://nodejs.org/dist/v0.11.9/node-v0.11.9-linux-x64.tar.gz)

16.losing hope I tried to compile node 0.11.9 from the sources

nvm uninstall 0.11.9
nvm install -s 0.11.9

17.and now ... everything works like a charm !! all tests passing green straight from @kkoopa's branch :-)

I knew "node.js" was bleeding edge so I am the only one to blame for spending so many hours on this ;-) but if you can help me understand what is going on here I will be grateful !!

Can we fix something so that these symbols get exported in the official node.js binaries or is it best practice to always compile node.js from source when using native extensions ?

@jeromew jeromew referenced this pull request in nodejs/node-v0.x-archive Jan 7, 2014
Closed

node 0.11.9 - missing symbols Integer::New, IsRegExp #6815

Contributor
jeromew commented Jan 8, 2014

Just for simplicity, the issue I created will lead you to another open issue on node.js. There is indeed a problem on the pre-packaged binaries and they are looking into it.

so until they fix this issue and if you want to test @kkoopa's branch on 0.11.x, compile node.js from the source and do not use pre-packaged versions

Member

@kkoopa - I've finished a bunch of packaging work (#245) and tagged v2.2.0. Next thing on my mind is testing your patch and getting it merged. Let me know if you might have time to update the branch (and ideally squash to a few commits), otherwise I can take a look at doing this when I have time (est 2-3 weeks).

Contributor
kkoopa commented Jan 14, 2014

Done.

Contributor
jeromew commented Feb 13, 2014

@koopa I think I have found an issue with the Nan branch for node-sqlite3.

I tracked a bug down which have led me to the following reproducible experiment.

var sqlite3 = require('sqlite3');
var db = new sqlite3.Database('db.sqlite');
db.get('select 2', function(err, res) {
  process.nextTick(function() {
    console.log(res);
  });
});
setTimeout(function() {}, 5000);

If I run this code with 0.10.22 with the master branch, it works as expected :

  • log 2 nearly immediatly
  • exit after 5 seconds

If I run this code with 0.11.9 with you NAN branch it does

  • log 2 after 5 seconds
  • exit

so it is as if node-sqlite3 was not returning to the main loop after calling the callback. The nextTick is registered but only called when the setTimeout wakes up.

do you have an idea of what could be the problem here ? It seems that node-sqlite does not use the NanAsync helpers I don't know if this could be the problem here.

Contributor
kkoopa commented Feb 13, 2014

You're starving the event loop or something. Use setImmediate instead of process.nexttick.

var sqlite3 = require('sqlite3');
require('set-immediate');  // needed for antique versions of node, which don't have setImmediate
var db = new sqlite3.Database('db.sqlite');
db.get('select 2', function(err, res) {
  setImmediate(function() {
    console.log(res);
  });
});
setTimeout(function() {}, 5000);
Contributor
jeromew commented Feb 13, 2014

ok I see. I will look into the 'starving event loop' concept.

I tried the same with node 0.10.22 + Nan branch and the event loop is not starving so the difference may be in node.

The issue is a bit more complex than just using set-immediate. Here is a little more complex example with the same problem :

var sqlite3 = require('sqlite3');
var Promise = require('promise');

var db = new sqlite3.Database('db.sqlite');
db.get = Promise.denodeify(db.get);

db.get('select 1').then(function(res) {
  console.log(res);
});

setTimeout(function(){}, 5000);
  • node 0.10.22 + Nan branch => OK early res logged
  • node 0.11.9 + Nan branch => KO res logged at event loop wake up

In fact, I am tracking down the issue in my server since I upgradred to promise version 4.0.

  • promise 4.0 uses require('asap') to async the resolution, which makes a nextTick
  • promise 3.2 (previous) uses setImmediate

@kkoopa where do you think such an issue should be dealt with ? There is a different behaviour between (node 0.10.22 + nan) and (node 0.11.9 + nan) regarding to the event loop. Libraries such as promise only highlight such difference and can make a server freeze if no other event wakes the event loop.

Is a a common thing to have a setInterval heartbeat in a server to make sure the event loop is not starving ?

I'll cross post this issue on promise

Contributor
jeromew commented Feb 13, 2014

Let me also just add that when using a common async function like readFile the problem does not appear

var fs = require('fs');
fs.readFile('myFile', function(err, res) {
  process.nextTick(function() {
    console.log(res);
  });
});
setTimeout(function() {}, 5000);

This correctly show the file immediatly with node 0.11.9. So we could maybe say that a 'regular' async library should not hold the event loop like node-sqlite3 does in the 0.11.9 + nan case.

@jeromew jeromew referenced this pull request in then/promise Feb 13, 2014
Closed

event loop starving with promise 4.0 #29

Contributor
kkoopa commented Feb 13, 2014

I honestly don't have a thorough understanding of all the event-loop intricacies. process.nextTick and setImmediate have been changed back and forth several times over node versions. Timings and such may also have been changed in 0.11, it seems more restrictive, but I don't know what actually is up. I don't think nan has influence in this, as it does not do much more than trivial code generation. Is it possible to hack up something without any addons that works in 0.10 but not in 0.11? That would eliminate nan from the equation.

I see you did that just now. This indicates that the binding may need some updating. Don't know where though. It is executing more or less identical code in 0.10 and 0.11. Maybe node 0.11 requires something extra in the binding to get the intended behavior?

Contributor
jeromew commented Feb 13, 2014

I started hunting this bug in my server yesterday. The only thing I saw that maybe could help is that in the nan port you are not using the https://github.com/rvagg/nan#api_nan_async_worker helpers while node-sqlite3 seem to be using 'legacy' code in async.h.

but I am far from knowledgeable on nan so it might not be a good hint and I have no idea if it is easy to replace this code with the nan helpers.

do you have an idea if it would be easy or not to change this node-sqlite3 part of the code to follow nan's conventions (or if it even makes any sense) ?

Contributor
kkoopa commented Feb 13, 2014

The async helpers in NAN are just helpers, nice to have when writing new code, but not necessary. Nothing significant should have changed in the libuv API between 0.10 and 0.11 and there is only one version of the async helper code in nan, not one for each version, as is the case for the necessary parts of nan.
Assuming the code using libuv is correct, it should work as is. It seems it was correct, at least for 0.10. I didn't make node-sqlite3 use the async helpers as that would have constituted even more rewriting and I was afraid something would get messed up in the conversion process. Less changes gives less potential for new bugs.

@rvagg Have you seen any of this in leveldown or other projects?

Contributor
kkoopa commented Feb 13, 2014

This seems vaguely familiar in that adding a call to console.log after process.nextTick makes the next tick happen.

rvagg commented Feb 13, 2014

sorry I'm not following the details here except to say that it's unlikely to be a NAN-specific problem since it's just effectively a wrapper around what's already there; plus you have no way of testing a non-NAN version in 0.11.

GC has changed a lot in the V8 used in 0.11 and I've experienced objects being aggressively removed in 0.11 such that callbacks don't get fired even though they are triggered. You have to be really careful with what you assign as Persistent, any slip there can result in callbacks becoming useless (and you won't see any kind of error!). Any time you need to keep an object/function around beyond a single C++ function that doesn't pass it back to JS you should be using a Persistent, there may be situations where it hasn't been a problem at all in 0.10 but now will be in 0.11.

Contributor
jeromew commented Feb 14, 2014

Hello, thanks @kkoopa & @rvagg

So I digged a little deeper in node-sqlite3 and stumbled upon this interesting article on how to write extensions: http://blog.trevnorris.com/2013/07/node-with-threads.html

The following fix in node-sqlite3 fixes the problem for me so I guess that MakeCallback is indeed the correct way to go back to js land.

--- a/src/macros.h
+++ b/src/macros.h
@@ -104,7 +104,7 @@ const char* sqlite_authorizer_string(int type);

 #define TRY_CATCH_CALL(context, callback, argc, argv)                          \
 {   TryCatch try_catch;                                                        \
-    (callback)->Call((context), (argc), (argv));                               \
+    MakeCallback((context), (callback), (argc), (argv));                       \
     if (try_catch.HasCaught()) {                                               \
         FatalException(try_catch);                                             \
     }                                                                          }

I sent a PR on @kkoopa's branch. Maybe you can confirm that it works in your tests also.
please note that I did not check if this is backwards compatible with 0.10.x

rvagg commented Feb 14, 2014

if you're using a recent version of NAN then anything that uses NanCallback is already going through MakeCallback if you use callback->Call(args);

Contributor
jeromew commented Feb 14, 2014

node-sqlite3'nan branch is using nan 0.8.0 but there is no trace of using NanCallback in the code.

I think that node-sqlite3 needs more nan work to use NanCallback because of the current prototype TRY_CATCH_CALL(context, callback, argc, argv) (NanCallback does not have a context and uses the v8::Context::GetCurrent()->Global() directly). It would be a lot better to use NanCallback here because it does a lot more house cleaning than the current code (Dispose, Persistent,..)

@koopa do you think you could have a look into this on your branch ? I did make the 'tip of fingers' change from Call to MakeCallback but the whole nan-macro-inside-node-sqlite3-macro's thing is wrangling my head for now ;-)

Contributor
kkoopa commented Feb 14, 2014

Great spelunking! This is excellent! I'll get on top of it. Still won't rewrite it to use nan async, but I should be able to hack something up with this. IIRC, MakeCallback has the same syntax since 0.8, some changes needed for 0.6 and less (nan handles all of this when using its async routines). I see sqlite3 isn't testing against 0.6. I'll assume it's not meant to be supported, so I won't bother with that either.

Contributor
kkoopa commented Feb 14, 2014

That was it. Now it works as intended. Great job hunting the error down.

@Mithgol Mithgol referenced this pull request Mar 27, 2014
Closed

Rebuild failure #265

@vincentbernat vincentbernat added a commit to vincentbernat/dashkiosk that referenced this pull request Apr 10, 2014
@vincentbernat vincentbernat travis: no support for node < 0.10, no support for node >= 0.11
For node < 0.10, some dependencies cannot be solved. We really don't
care. For node >= 0.11, it seems that there are problems with sqlite3
extension. Didn't dig much, but may be related:
 mapbox/node-sqlite3#184
6317823
@springmeyer springmeyer merged commit 4267d54 into mapbox:master Jun 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Member

This has now been merged! Node-sqlite >= v2.2.4 will now support node >= v0.11.13 and above thanks to the hard work of the Nan contributors.

rvagg commented Jun 20, 2014

To clarify: as of nan@1.0.0 we removed support for all 0.11 versions prior to 0.11.13, the changes were too traumatic to maintain anything else. 0.8, 0.10 (and even 0.6 I believe) are all fine though.

Member

@rvagg - sounds good. Edited my statement above to clarify.

dimsuz commented Jun 27, 2014

Maybe README.md needs to be updated then?
It still says "Depends: Node.js v0.10.x or v0.8.x"

Collaborator
Mithgol commented Jun 27, 2014

@dimsuz Good idea. I've updated README.md (commit 28e5774).

dimsuz commented Jun 27, 2014

@Mithgol thanks! At last it is possible to use it with recent node-webkit! Will try to migrate this evening :)

Collaborator
Mithgol commented Jun 28, 2014

@dimsuz Some changes are still necessary for such migration to be successful. See #308 for details.

dimsuz commented Jun 28, 2014

@Mithgol thank you very much, I will follow up

gpetrov commented Jul 7, 2014

@Mithgol - So are we OK to build for NW 0.9.2? I still got plenty of errors ...

Collaborator
Mithgol commented Jul 8, 2014

@gpetrov No, it's not OK to build for NW 0.9.2.

Node-webkit v0.9.2 is based on Node.js v0.11.9, and that version is not supported by node-sqlite3.

In the v0.11.x branch of Node.js we support only v0.11.13 and newer (that's what nan@1.0.0 does as @rvagg explained in a comment above).

It's likely that node-webkit 0.9.2 is not to be supported by node-sqlite3 at all.

Collaborator
Mithgol commented Jul 8, 2014

P. S.   Currently even a support for node-webkit 0.10.0-rc1 requires installing node-sqlite3 from the experimental travis-refactor branch, see #311 (comment) for details.

(That branch is likely to be eventually landed to the master and published as a new version of the sqlite3 package, but not right now.)

gpetrov commented Jul 8, 2014

@Mithgol Yes I found that out the hard way. However node-sqlite3 has just landed version 2.2.4 which should build just fine in node 0.11.

However nw-gyp is now giving problems nwjs/nw-gyp#32

Hope @rogerwang will solve them soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment