search

josephg / node-foundationdb

Modern Node.js FoundationDB bindings
Other
115 stars 17 forks source link

Callback API seems to be broken #45

Closed ex3ndr closed 4 years ago

ex3ndr commented 4 years ago

I am trying to use callback api instead of promise one and they all are never called.

josephg commented 4 years ago

The callback api is supported by the native module but the node library which wraps it only supports promises. Why do you want to use callbacks? Like, what problem do you have that that solves?

ex3ndr commented 4 years ago

Well, they are still exists in typings everywhere. I was thinking to benchmark and improve performance and wanted to rule out promises and it seems that they are not working at all.

josephg commented 4 years ago

Oh rad - I'd be very curious to see the results of that benchmark.

The typings should be correct. Taking a look at the code, tn.get() and a few other transaction methods should support callbacks. Hm, my test program with it is failing though. I'm taking a look...

josephg commented 4 years ago

Oh yeah this is mega broken. This call to napi_call_function is returning napi_invalid_arg and we're just throwing away the error. As a result, the callback is indeed never called. I'll fix it up when I have some time later today.

Obv the callback oriented API is just never exercised in my tests, because I don't use it. In the long run I might just rip out this code path - but I'll fix it up first so you can run some benchmarks.

josephg commented 4 years ago

Ok if you checkout master it should be working now - although the API is pretty awkward to use with callbacks because there's no helper wrapper to do the standard transaction retry loop logic. Let me know how you get on with it!