From c51bd7734028ebf49b8669229663778c758fc0c4 Mon Sep 17 00:00:00 2001
From: Seph Gentle <me@josephg.com>
Date: Fri, 22 May 2020 21:10:59 +1000
Subject: [PATCH] Fixed callback handler in native code. Fixes #45

---
 src/future.cpp      | 22 ++++++++++++++++------
 src/transaction.cpp |  1 +
 test/kv.ts          | 27 +++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/src/future.cpp b/src/future.cpp
index e576448..38f7727 100644
--- a/src/future.cpp
+++ b/src/future.cpp
@@ -4,6 +4,8 @@
 #include "utils.h"
 #include "future.h"
 
+// #include <cstdio>
+
 // #include <v8.h>
 
 // #include "Version.h"
@@ -178,13 +180,21 @@ MaybeValue fdbFutureToCallback(napi_env env, FDBFuture *f, napi_value cbFunc, Ex
     NAPI_OK_OR_RETURN_STATUS(env, napi_get_reference_value(env, ctx->cbFunc, &callback));
     NAPI_OK_OR_RETURN_STATUS(env, napi_reference_unref(env, ctx->cbFunc, NULL));
 
-    napi_value args[2] = {}; // (err, value).
-    if (errcode != 0) NAPI_OK_OR_RETURN_STATUS(env, wrap_fdb_error(env, errcode, &args[0]));
-    else if (value.status != napi_ok) NAPI_OK_OR_RETURN_STATUS(env, napi_get_and_clear_last_exception(env, &args[0]));
-    else args[1] = value.value;
+    size_t argc = 1; // In case of error we just won't populate argv[1].
+    napi_value argv[2] = {}; // (err, value).
+
+    if (errcode != 0) NAPI_OK_OR_RETURN_STATUS(env, wrap_fdb_error(env, errcode, &argv[0]));
+    else if (value.status != napi_ok) NAPI_OK_OR_RETURN_STATUS(env, napi_get_and_clear_last_exception(env, &argv[0]));
+    else {
+      argc = 2;
+      NAPI_OK_OR_RETURN_STATUS(env, napi_get_undefined(env, &argv[0]));
+      argv[1] = value.value;
+    }
 
-    // If this throws it'll bubble up to the node uncaught exception handler, which is what we want.
-    napi_call_function(env, NULL, callback, 2, args, NULL);
+    napi_value global;
+    NAPI_OK_OR_RETURN_STATUS(env, napi_get_global(env, &global));
+    // We're discarding the return value here.
+    NAPI_OK_OR_RETURN_STATUS(env, napi_call_function(env, global, callback, argc, argv, NULL));
 
     return napi_ok;
   });
diff --git a/src/transaction.cpp b/src/transaction.cpp
index 5a0a51b..da2426f 100644
--- a/src/transaction.cpp
+++ b/src/transaction.cpp
@@ -22,6 +22,7 @@
  */
 
 #include <cstdlib>
+// #include <cstdio>
 #include <cassert>
 
 #include "options.h"
diff --git a/test/kv.ts b/test/kv.ts
index a516994..3da2c77 100644
--- a/test/kv.ts
+++ b/test/kv.ts
@@ -370,6 +370,33 @@ withEachDb(db => describe('key value functionality', () => {
     })
   })
 
+  describe('callback API', () => {
+    // Unless benchmarking shows a significant difference, this will be removed
+    // in a future version of this library.
+    it('allows tn.get() with a callback', async () => {
+      let called = false
+      await db.doTransaction(async tn => {
+        tn.set('xxx', 'hi')
+
+        // Ok now fetch it. I'm wrapping this in an awaited promise so we don't
+        // commit the transaction before .get() has resolved.
+        await new Promise((resolve, reject) => {
+          tn.get('xxx', (err, data) => {
+            try {
+              assert(!called)
+              called = true
+              if (err) throw err
+              assert.deepStrictEqual(data, Buffer.from('hi'))
+              resolve()
+            } catch (e) { reject(e) }
+          })
+        })
+      })
+      assert(called)
+    })
+    
+  })
+
   describe('regression', () => {
     it('does not trim off the end of a string', async () => {
       // https://github.com/josephg/node-foundationdb/issues/40