From 81778f0ed4b2df3baea704fab9ef6b4030e67220 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sun, 15 Mar 2026 12:14:08 +0000 Subject: [PATCH 1/3] Fix child QueryResult lifetime after parent close --- src_cpp/include/node_query_result.h | 18 ++++++++++--- src_cpp/node_query_result.cpp | 40 ++++++++++++++++++++++++----- test/test_sync_api.js | 15 +++++++++++ 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src_cpp/include/node_query_result.h b/src_cpp/include/node_query_result.h index b9ee4db..1f33773 100644 --- a/src_cpp/include/node_query_result.h +++ b/src_cpp/include/node_query_result.h @@ -20,6 +20,8 @@ class NodeQueryResult : public Napi::ObjectWrap { static Napi::Object Init(Napi::Env env, Napi::Object exports); explicit NodeQueryResult(const Napi::CallbackInfo& info); void SetQueryResult(QueryResult* queryResult, bool isOwned); + void SetOwnedQueryResult(std::unique_ptr queryResult); + std::unique_ptr DetachNextQueryResult(); ~NodeQueryResult() override; private: @@ -43,8 +45,8 @@ class NodeQueryResult : public Napi::ObjectWrap { private: QueryResult* queryResult = nullptr; + std::unique_ptr ownedQueryResult = nullptr; std::unique_ptr> columnNames = nullptr; - bool isOwned = false; }; enum GetColumnMetadataType { DATA_TYPE, NAME }; @@ -147,12 +149,21 @@ class NodeQueryResultGetNextQueryResultAsyncWorker : public Napi::AsyncWorker { void Execute() override { try { - auto nextResult = currQueryResult->queryResult->getNextQueryResult(); + nextOwnedResult = currQueryResult->DetachNextQueryResult(); + auto nextResult = + nextOwnedResult ? nextOwnedResult.get() : currQueryResult->queryResult->getNextQueryResult(); + if (nextResult == nullptr) { + return; + } if (!nextResult->isSuccess()) { SetError(nextResult->getErrorMessage()); return; } - nextQueryResult->SetQueryResult(nextResult, false); + if (nextOwnedResult) { + nextQueryResult->SetOwnedQueryResult(std::move(nextOwnedResult)); + } else { + nextQueryResult->SetQueryResult(nextResult, false); + } } catch (const std::exception& exc) { SetError(std::string(exc.what())); } @@ -165,6 +176,7 @@ class NodeQueryResultGetNextQueryResultAsyncWorker : public Napi::AsyncWorker { private: NodeQueryResult* currQueryResult; NodeQueryResult* nextQueryResult; + std::unique_ptr nextOwnedResult; }; class NodeQueryResultGetQuerySummaryAsyncWorker : public Napi::AsyncWorker { diff --git a/src_cpp/node_query_result.cpp b/src_cpp/node_query_result.cpp index 24c1822..ee72b89 100644 --- a/src_cpp/node_query_result.cpp +++ b/src_cpp/node_query_result.cpp @@ -38,9 +38,27 @@ NodeQueryResult::~NodeQueryResult() { this->Close(); } +void NodeQueryResult::SetOwnedQueryResult(std::unique_ptr queryResult) { + Close(); + ownedQueryResult = std::move(queryResult); + this->queryResult = ownedQueryResult.get(); +} + void NodeQueryResult::SetQueryResult(QueryResult* queryResult, bool isOwned) { + Close(); + if (isOwned) { + ownedQueryResult.reset(queryResult); + this->queryResult = ownedQueryResult.get(); + return; + } this->queryResult = queryResult; - this->isOwned = isOwned; +} + +std::unique_ptr NodeQueryResult::DetachNextQueryResult() { + if (ownedQueryResult == nullptr) { + return nullptr; + } + return ownedQueryResult->moveNextResult(); } void NodeQueryResult::ResetIterator(const Napi::CallbackInfo& info) { @@ -90,13 +108,22 @@ Napi::Value NodeQueryResult::GetNextQueryResultSync(const Napi::CallbackInfo& in Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - auto nextResult = this->queryResult->getNextQueryResult(); + auto nextOwnedResult = DetachNextQueryResult(); + auto nextResult = + nextOwnedResult ? nextOwnedResult.get() : this->queryResult->getNextQueryResult(); + if (nextResult == nullptr) { + return env.Undefined(); + } if (!nextResult->isSuccess()) { Napi::Error::New(env, nextResult->getErrorMessage()).ThrowAsJavaScriptException(); } auto nodeQueryResult = Napi::ObjectWrap::Unwrap(info[0].As()); - nodeQueryResult->SetQueryResult(nextResult, false); + if (nextOwnedResult) { + nodeQueryResult->SetOwnedQueryResult(std::move(nextOwnedResult)); + } else { + nodeQueryResult->SetQueryResult(nextResult, false); + } } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -235,8 +262,7 @@ void NodeQueryResult::Close(const Napi::CallbackInfo& info) { } void NodeQueryResult::Close() { - if (this->isOwned) { - delete this->queryResult; - this->queryResult = nullptr; - } + columnNames.reset(); + ownedQueryResult.reset(); + queryResult = nullptr; } diff --git a/test/test_sync_api.js b/test/test_sync_api.js index 2566722..b1fc7b1 100644 --- a/test/test_sync_api.js +++ b/test/test_sync_api.js @@ -27,6 +27,21 @@ describe("Query execution", function () { } }); + it("should keep child query results usable after closing the parent result", function () { + const queryResults = conn.querySync("RETURN 1 AS first; RETURN 2 AS second;"); + const parentResult = queryResults[0]; + const childResult = queryResults[1]; + + const firstChildTuple = childResult.getNextSync(); + assert.equal(firstChildTuple["second"], 2); + parentResult.close(); + + childResult.resetIterator(); + const secondChildTuple = childResult.getNextSync(); + assert.equal(secondChildTuple["second"], 2); + childResult.close(); + }); + it("should execute a prepared statement synchronously", function () { const preparedStatement = conn.prepareSync( "RETURN $a as id" From 7def8b1462ad30cd311bd9a46ee6e03dc8245b02 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sun, 15 Mar 2026 15:22:04 +0000 Subject: [PATCH 2/3] Add QueryResult close-order regression tests --- test/test_connection.js | 36 ++++++++++++++++++++++++++++++++++++ test/test_sync_api.js | 19 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/test/test_connection.js b/test/test_connection.js index 34345ad..b1a51bb 100644 --- a/test/test_connection.js +++ b/test/test_connection.js @@ -168,6 +168,42 @@ describe("Query", function () { assert.deepEqual(results, [[{ 1: 1 }], [{ 2: 2 }], [{ 3: 3 }]]); }); + it("should keep child query results usable after closing the parent result", async function () { + const queryResults = await conn.query("RETURN 1 AS first; RETURN 2 AS second;"); + const parentResult = queryResults[0]; + const childResult = queryResults[1]; + + const firstChildTuple = await childResult.getNext(); + assert.equal(firstChildTuple["second"], 2); + parentResult.close(); + + childResult.resetIterator(); + const secondChildTuple = await childResult.getNext(); + assert.equal(secondChildTuple["second"], 2); + childResult.close(); + }); + + it("should keep deeper child query results usable after closing earlier results", async function () { + const queryResults = await conn.query(` + RETURN 1 AS first; + RETURN 2 AS second; + RETURN 3 AS third; + `); + const firstResult = queryResults[0]; + const secondResult = queryResults[1]; + const thirdResult = queryResults[2]; + + const firstThirdTuple = await thirdResult.getNext(); + assert.equal(firstThirdTuple["third"], 3); + firstResult.close(); + secondResult.close(); + + thirdResult.resetIterator(); + const secondThirdTuple = await thirdResult.getNext(); + assert.equal(secondThirdTuple["third"], 3); + thirdResult.close(); + }); + it("should throw error if one of the multiple queries is invalid", async function () { try { await conn.query(` diff --git a/test/test_sync_api.js b/test/test_sync_api.js index b1fc7b1..8fa8991 100644 --- a/test/test_sync_api.js +++ b/test/test_sync_api.js @@ -42,6 +42,25 @@ describe("Query execution", function () { childResult.close(); }); + it("should keep deeper child query results usable after closing earlier results", function () { + const queryResults = conn.querySync( + "RETURN 1 AS first; RETURN 2 AS second; RETURN 3 AS third;" + ); + const firstResult = queryResults[0]; + const secondResult = queryResults[1]; + const thirdResult = queryResults[2]; + + const firstThirdTuple = thirdResult.getNextSync(); + assert.equal(firstThirdTuple["third"], 3); + firstResult.close(); + secondResult.close(); + + thirdResult.resetIterator(); + const secondThirdTuple = thirdResult.getNextSync(); + assert.equal(secondThirdTuple["third"], 3); + thirdResult.close(); + }); + it("should execute a prepared statement synchronously", function () { const preparedStatement = conn.prepareSync( "RETURN $a as id" From ff7b82a8e7f0f0960226889ddc889dab54df8fa3 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Mon, 16 Mar 2026 08:51:24 +0000 Subject: [PATCH 3/3] Strengthen NodeQueryResult ownership model --- src_cpp/include/node_connection.h | 19 ++--- src_cpp/include/node_query_result.h | 110 ++++++++++++++++++++-------- src_cpp/node_connection.cpp | 22 +++--- src_cpp/node_query_result.cpp | 103 +++++++++++++++----------- src_js/connection.js | 9 ++- test/test_connection.js | 14 ++++ test/test_sync_api.js | 14 ++++ 7 files changed, 194 insertions(+), 97 deletions(-) diff --git a/src_cpp/include/node_connection.h b/src_cpp/include/node_connection.h index 1c650cb..b5f4bfd 100644 --- a/src_cpp/include/node_connection.h +++ b/src_cpp/include/node_connection.h @@ -99,11 +99,11 @@ class ConnectionExecuteAsyncWorker : public Napi::AsyncWorker { try { auto result = connection - ->executeWithParamsWithID(preparedStatement.get(), std::move(params), queryID) - .release(); - nodeQueryResult->SetQueryResult(result, true); - if (!result->isSuccess()) { - SetError(result->getErrorMessage()); + ->executeWithParamsWithID(preparedStatement.get(), std::move(params), queryID); + auto* resultRaw = result.get(); + nodeQueryResult->AdoptQueryResult(std::move(result)); + if (!resultRaw->isSuccess()) { + SetError(resultRaw->getErrorMessage()); return; } } catch (const std::exception& exc) { @@ -155,10 +155,11 @@ class ConnectionQueryAsyncWorker : public Napi::AsyncWorker { progressBar->toggleProgressBarPrinting(true); } try { - auto result = connection->queryWithID(statement, queryID).release(); - nodeQueryResult->SetQueryResult(result, true); - if (!result->isSuccess()) { - SetError(result->getErrorMessage()); + auto result = connection->queryWithID(statement, queryID); + auto* resultRaw = result.get(); + nodeQueryResult->AdoptQueryResult(std::move(result)); + if (!resultRaw->isSuccess()) { + SetError(resultRaw->getErrorMessage()); } } catch (const std::exception& exc) { SetError(std::string(exc.what())); diff --git a/src_cpp/include/node_query_result.h b/src_cpp/include/node_query_result.h index 1f33773..ae71b27 100644 --- a/src_cpp/include/node_query_result.h +++ b/src_cpp/include/node_query_result.h @@ -5,6 +5,8 @@ #include "node_util.h" #include "planner/operator/logical_plan.h" #include "processor/result/factorized_table.h" +#include +#include #include using namespace lbug::processor; @@ -18,9 +20,9 @@ class NodeQueryResult : public Napi::ObjectWrap { public: static Napi::Object Init(Napi::Env env, Napi::Object exports); + static Napi::Object NewInstance(Napi::Env env, std::unique_ptr queryResult); explicit NodeQueryResult(const Napi::CallbackInfo& info); - void SetQueryResult(QueryResult* queryResult, bool isOwned); - void SetOwnedQueryResult(std::unique_ptr queryResult); + void AdoptQueryResult(std::unique_ptr queryResult); std::unique_ptr DetachNextQueryResult(); ~NodeQueryResult() override; @@ -42,11 +44,16 @@ class NodeQueryResult : public Napi::ObjectWrap { void PopulateColumnNames(); void Close(const Napi::CallbackInfo& info); void Close(); + QueryResult& GetQueryResult() const; + void AcquireAsyncUse(); + void ReleaseAsyncUse(); + void ThrowIfAsyncOperationInFlight(const char* operation) const; private: - QueryResult* queryResult = nullptr; + static Napi::FunctionReference constructor; std::unique_ptr ownedQueryResult = nullptr; std::unique_ptr> columnNames = nullptr; + std::atomic activeAsyncUses = 0; }; enum GetColumnMetadataType { DATA_TYPE, NAME }; @@ -55,14 +62,17 @@ class NodeQueryResultGetColumnMetadataAsyncWorker : public Napi::AsyncWorker { public: NodeQueryResultGetColumnMetadataAsyncWorker(Napi::Function& callback, NodeQueryResult* nodeQueryResult, GetColumnMetadataType type) - : AsyncWorker(callback), nodeQueryResult(nodeQueryResult), type(type) {} + : AsyncWorker(callback), nodeQueryResult(nodeQueryResult), type(type) { + nodeQueryResult->AcquireAsyncUse(); + nodeQueryResult->Ref(); + } ~NodeQueryResultGetColumnMetadataAsyncWorker() override = default; inline void Execute() override { try { if (type == GetColumnMetadataType::DATA_TYPE) { - auto columnDataTypes = nodeQueryResult->queryResult->getColumnDataTypes(); + auto columnDataTypes = nodeQueryResult->GetQueryResult().getColumnDataTypes(); result = std::vector(columnDataTypes.size()); for (auto i = 0u; i < columnDataTypes.size(); ++i) { result[i] = columnDataTypes[i].toString(); @@ -82,10 +92,16 @@ class NodeQueryResultGetColumnMetadataAsyncWorker : public Napi::AsyncWorker { for (auto i = 0u; i < result.size(); ++i) { nodeResult.Set(i, result[i]); } + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); Callback().Call({env.Null(), nodeResult}); } - inline void OnError(Napi::Error const& error) override { Callback().Call({error.Value()}); } + inline void OnError(Napi::Error const& error) override { + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); + Callback().Call({error.Value()}); + } private: NodeQueryResult* nodeQueryResult; @@ -96,16 +112,20 @@ class NodeQueryResultGetColumnMetadataAsyncWorker : public Napi::AsyncWorker { class NodeQueryResultGetNextAsyncWorker : public Napi::AsyncWorker { public: NodeQueryResultGetNextAsyncWorker(Napi::Function& callback, NodeQueryResult* nodeQueryResult) - : AsyncWorker(callback), nodeQueryResult(nodeQueryResult) {} + : AsyncWorker(callback), nodeQueryResult(nodeQueryResult) { + nodeQueryResult->AcquireAsyncUse(); + nodeQueryResult->Ref(); + } ~NodeQueryResultGetNextAsyncWorker() override = default; inline void Execute() override { try { - if (!nodeQueryResult->queryResult->hasNext()) { + auto& queryResult = nodeQueryResult->GetQueryResult(); + if (!queryResult.hasNext()) { cppTuple.reset(); } - cppTuple = nodeQueryResult->queryResult->getNext(); + cppTuple = queryResult.getNext(); } catch (const std::exception& exc) { SetError(std::string(exc.what())); } @@ -114,24 +134,35 @@ class NodeQueryResultGetNextAsyncWorker : public Napi::AsyncWorker { inline void OnOK() override { auto env = Env(); if (cppTuple == nullptr) { + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); Callback().Call({env.Null(), env.Undefined()}); return; } Napi::Object nodeTuple = Napi::Object::New(env); try { - auto columnNames = nodeQueryResult->queryResult->getColumnNames(); + auto columnNames = nodeQueryResult->GetQueryResult().getColumnNames(); for (auto i = 0u; i < cppTuple->len(); ++i) { Napi::Value value = Util::ConvertToNapiObject(*cppTuple->getValue(i), env); nodeTuple.Set(columnNames[i], value); } } catch (const std::exception& exc) { auto napiError = Napi::Error::New(env, exc.what()); + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); Callback().Call({napiError.Value(), env.Undefined()}); + return; } + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); Callback().Call({env.Null(), nodeTuple}); } - inline void OnError(Napi::Error const& error) override { Callback().Call({error.Value()}); } + inline void OnError(Napi::Error const& error) override { + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); + Callback().Call({error.Value()}); + } private: NodeQueryResult* nodeQueryResult; @@ -140,42 +171,48 @@ class NodeQueryResultGetNextAsyncWorker : public Napi::AsyncWorker { class NodeQueryResultGetNextQueryResultAsyncWorker : public Napi::AsyncWorker { public: - NodeQueryResultGetNextQueryResultAsyncWorker(Napi::Function& callback, - NodeQueryResult* currentQueryResult, NodeQueryResult* nextQueryResult) - : AsyncWorker(callback), currQueryResult(currentQueryResult), - nextQueryResult(nextQueryResult) {} + NodeQueryResultGetNextQueryResultAsyncWorker( + Napi::Function& callback, NodeQueryResult* currentQueryResult) + : AsyncWorker(callback), currQueryResult(currentQueryResult) { + currQueryResult->AcquireAsyncUse(); + currQueryResult->Ref(); + } ~NodeQueryResultGetNextQueryResultAsyncWorker() override = default; void Execute() override { try { nextOwnedResult = currQueryResult->DetachNextQueryResult(); - auto nextResult = - nextOwnedResult ? nextOwnedResult.get() : currQueryResult->queryResult->getNextQueryResult(); - if (nextResult == nullptr) { - return; - } - if (!nextResult->isSuccess()) { - SetError(nextResult->getErrorMessage()); + if (nextOwnedResult == nullptr) { return; } - if (nextOwnedResult) { - nextQueryResult->SetOwnedQueryResult(std::move(nextOwnedResult)); - } else { - nextQueryResult->SetQueryResult(nextResult, false); + if (!nextOwnedResult->isSuccess()) { + SetError(nextOwnedResult->getErrorMessage()); } } catch (const std::exception& exc) { SetError(std::string(exc.what())); } } - void OnOK() override { Callback().Call({Env().Null()}); } + void OnOK() override { + auto env = Env(); + currQueryResult->ReleaseAsyncUse(); + currQueryResult->Unref(); + if (nextOwnedResult == nullptr) { + Callback().Call({env.Null(), env.Undefined()}); + return; + } + Callback().Call({env.Null(), NodeQueryResult::NewInstance(env, std::move(nextOwnedResult))}); + } - void OnError(Napi::Error const& error) override { Callback().Call({error.Value()}); } + void OnError(Napi::Error const& error) override { + currQueryResult->ReleaseAsyncUse(); + currQueryResult->Unref(); + Callback().Call({error.Value()}); + } private: NodeQueryResult* currQueryResult; - NodeQueryResult* nextQueryResult; std::unique_ptr nextOwnedResult; }; @@ -183,13 +220,16 @@ class NodeQueryResultGetQuerySummaryAsyncWorker : public Napi::AsyncWorker { public: NodeQueryResultGetQuerySummaryAsyncWorker(Napi::Function& callback, NodeQueryResult* nodeQueryResult) - : AsyncWorker(callback), nodeQueryResult(nodeQueryResult) {} + : AsyncWorker(callback), nodeQueryResult(nodeQueryResult) { + nodeQueryResult->AcquireAsyncUse(); + nodeQueryResult->Ref(); + } ~NodeQueryResultGetQuerySummaryAsyncWorker() override = default; inline void Execute() override { try { - auto querySummary = nodeQueryResult->queryResult->getQuerySummary(); + auto querySummary = nodeQueryResult->GetQueryResult().getQuerySummary(); result["compilingTime"] = querySummary->getCompilingTime(); result["executionTime"] = querySummary->getExecutionTime(); } catch (const std::exception& exc) { @@ -203,10 +243,16 @@ class NodeQueryResultGetQuerySummaryAsyncWorker : public Napi::AsyncWorker { for (const auto& [key, value] : result) { nodeResult.Set(key, Napi::Number::New(env, value)); } + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); Callback().Call({env.Null(), nodeResult}); } - inline void OnError(Napi::Error const& error) override { Callback().Call({error.Value()}); } + inline void OnError(Napi::Error const& error) override { + nodeQueryResult->ReleaseAsyncUse(); + nodeQueryResult->Unref(); + Callback().Call({error.Value()}); + } private: NodeQueryResult* nodeQueryResult; diff --git a/src_cpp/node_connection.cpp b/src_cpp/node_connection.cpp index 4049039..a0090f5 100644 --- a/src_cpp/node_connection.cpp +++ b/src_cpp/node_connection.cpp @@ -113,10 +113,11 @@ Napi::Value NodeConnection::QuerySync(const Napi::CallbackInfo& info) { auto statement = info[0].As().Utf8Value(); auto nodeQueryResult = Napi::ObjectWrap::Unwrap(info[1].As()); try { - auto result = connection->query(statement).release(); - nodeQueryResult->SetQueryResult(result, true); - if (!result->isSuccess()) { - Napi::Error::New(env, result->getErrorMessage()).ThrowAsJavaScriptException(); + auto result = connection->query(statement); + auto* resultRaw = result.get(); + nodeQueryResult->AdoptQueryResult(std::move(result)); + if (!resultRaw->isSuccess()) { + Napi::Error::New(env, resultRaw->getErrorMessage()).ThrowAsJavaScriptException(); } } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); @@ -132,13 +133,12 @@ Napi::Value NodeConnection::ExecuteSync(const Napi::CallbackInfo& info) { auto nodeQueryResult = Napi::ObjectWrap::Unwrap(info[1].As()); try { auto params = Util::TransformParametersForExec(info[2].As()); - auto result = connection - ->executeWithParams(nodePreparedStatement->preparedStatement.get(), - std::move(params)) - .release(); - nodeQueryResult->SetQueryResult(result, true); - if (!result->isSuccess()) { - Napi::Error::New(env, result->getErrorMessage()).ThrowAsJavaScriptException(); + auto result = connection->executeWithParams(nodePreparedStatement->preparedStatement.get(), + std::move(params)); + auto* resultRaw = result.get(); + nodeQueryResult->AdoptQueryResult(std::move(result)); + if (!resultRaw->isSuccess()) { + Napi::Error::New(env, resultRaw->getErrorMessage()).ThrowAsJavaScriptException(); } } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); diff --git a/src_cpp/node_query_result.cpp b/src_cpp/node_query_result.cpp index ee72b89..258abe7 100644 --- a/src_cpp/node_query_result.cpp +++ b/src_cpp/node_query_result.cpp @@ -1,5 +1,6 @@ #include "include/node_query_result.h" +#include #include #include "include/node_util.h" @@ -7,6 +8,8 @@ using namespace lbug::main; +Napi::FunctionReference NodeQueryResult::constructor; + Napi::Object NodeQueryResult::Init(Napi::Env env, Napi::Object exports) { Napi::HandleScope scope(env); @@ -27,10 +30,20 @@ Napi::Object NodeQueryResult::Init(Napi::Env env, Napi::Object exports) { InstanceMethod("getQuerySummarySync", &NodeQueryResult::GetQuerySummarySync), InstanceMethod("close", &NodeQueryResult::Close)}); + constructor = Napi::Persistent(t); + constructor.SuppressDestruct(); exports.Set("NodeQueryResult", t); return exports; } +Napi::Object NodeQueryResult::NewInstance( + Napi::Env /*env*/, std::unique_ptr queryResult) { + auto obj = constructor.New({}); + auto* nodeQueryResult = Napi::ObjectWrap::Unwrap(obj); + nodeQueryResult->AdoptQueryResult(std::move(queryResult)); + return obj; +} + NodeQueryResult::NodeQueryResult(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) {} @@ -38,20 +51,10 @@ NodeQueryResult::~NodeQueryResult() { this->Close(); } -void NodeQueryResult::SetOwnedQueryResult(std::unique_ptr queryResult) { - Close(); +void NodeQueryResult::AdoptQueryResult(std::unique_ptr queryResult) { + ThrowIfAsyncOperationInFlight("replace"); + columnNames.reset(); ownedQueryResult = std::move(queryResult); - this->queryResult = ownedQueryResult.get(); -} - -void NodeQueryResult::SetQueryResult(QueryResult* queryResult, bool isOwned) { - Close(); - if (isOwned) { - ownedQueryResult.reset(queryResult); - this->queryResult = ownedQueryResult.get(); - return; - } - this->queryResult = queryResult; } std::unique_ptr NodeQueryResult::DetachNextQueryResult() { @@ -61,11 +64,33 @@ std::unique_ptr NodeQueryResult::DetachNextQueryResult() { return ownedQueryResult->moveNextResult(); } +QueryResult& NodeQueryResult::GetQueryResult() const { + if (ownedQueryResult == nullptr) { + throw std::runtime_error("Query result is closed."); + } + return *ownedQueryResult; +} + +void NodeQueryResult::AcquireAsyncUse() { + activeAsyncUses.fetch_add(1, std::memory_order_relaxed); +} + +void NodeQueryResult::ReleaseAsyncUse() { + activeAsyncUses.fetch_sub(1, std::memory_order_relaxed); +} + +void NodeQueryResult::ThrowIfAsyncOperationInFlight(const char* operation) const { + if (activeAsyncUses.load(std::memory_order_acquire) != 0) { + throw std::runtime_error(std::string("Cannot ") + operation + + " QueryResult while an async operation is in flight."); + } +} + void NodeQueryResult::ResetIterator(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - this->queryResult->resetIterator(); + GetQueryResult().resetIterator(); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -75,7 +100,7 @@ Napi::Value NodeQueryResult::HasNext(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - return Napi::Boolean::New(env, this->queryResult->hasNext()); + return Napi::Boolean::New(env, GetQueryResult().hasNext()); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -86,7 +111,7 @@ Napi::Value NodeQueryResult::HasNextQueryResult(const Napi::CallbackInfo& info) Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - return Napi::Boolean::New(env, this->queryResult->hasNextQueryResult()); + return Napi::Boolean::New(env, GetQueryResult().hasNextQueryResult()); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -96,10 +121,8 @@ Napi::Value NodeQueryResult::HasNextQueryResult(const Napi::CallbackInfo& info) Napi::Value NodeQueryResult::GetNextQueryResultAsync(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); Napi::HandleScope scope(env); - auto newQueryResult = Napi::ObjectWrap::Unwrap(info[0].As()); - auto callback = info[1].As(); - auto* asyncWorker = - new NodeQueryResultGetNextQueryResultAsyncWorker(callback, this, newQueryResult); + auto callback = info[0].As(); + auto* asyncWorker = new NodeQueryResultGetNextQueryResultAsyncWorker(callback, this); asyncWorker->Queue(); return info.Env().Undefined(); } @@ -109,21 +132,15 @@ Napi::Value NodeQueryResult::GetNextQueryResultSync(const Napi::CallbackInfo& in Napi::HandleScope scope(env); try { auto nextOwnedResult = DetachNextQueryResult(); - auto nextResult = - nextOwnedResult ? nextOwnedResult.get() : this->queryResult->getNextQueryResult(); - if (nextResult == nullptr) { + if (nextOwnedResult == nullptr) { return env.Undefined(); } - if (!nextResult->isSuccess()) { - Napi::Error::New(env, nextResult->getErrorMessage()).ThrowAsJavaScriptException(); - } - auto nodeQueryResult = - Napi::ObjectWrap::Unwrap(info[0].As()); - if (nextOwnedResult) { - nodeQueryResult->SetOwnedQueryResult(std::move(nextOwnedResult)); - } else { - nodeQueryResult->SetQueryResult(nextResult, false); + if (!nextOwnedResult->isSuccess()) { + Napi::Error::New(env, nextOwnedResult->getErrorMessage()) + .ThrowAsJavaScriptException(); + return env.Undefined(); } + return NewInstance(env, std::move(nextOwnedResult)); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -134,7 +151,7 @@ Napi::Value NodeQueryResult::GetNumTuples(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - return Napi::Number::New(env, this->queryResult->getNumTuples()); + return Napi::Number::New(env, GetQueryResult().getNumTuples()); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -154,10 +171,11 @@ Napi::Value NodeQueryResult::GetNextSync(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - if (!this->queryResult->hasNext()) { + auto& queryResult = GetQueryResult(); + if (!queryResult.hasNext()) { return env.Null(); } - auto cppTuple = this->queryResult->getNext(); + auto cppTuple = queryResult.getNext(); Napi::Object nodeTuple = Napi::Object::New(env); PopulateColumnNames(); for (auto i = 0u; i < cppTuple->len(); ++i) { @@ -185,7 +203,7 @@ Napi::Value NodeQueryResult::GetColumnDataTypesSync(const Napi::CallbackInfo& in Napi::Env env = info.Env(); Napi::HandleScope scope(env); try { - auto columnDataTypes = this->queryResult->getColumnDataTypes(); + auto columnDataTypes = GetQueryResult().getColumnDataTypes(); Napi::Array nodeColumnDataTypes = Napi::Array::New(env, columnDataTypes.size()); for (auto i = 0u; i < columnDataTypes.size(); ++i) { nodeColumnDataTypes.Set(i, Napi::String::New(env, columnDataTypes[i].toString())); @@ -227,8 +245,7 @@ void NodeQueryResult::PopulateColumnNames() { if (this->columnNames != nullptr) { return; } - this->columnNames = - std::make_unique>(this->queryResult->getColumnNames()); + this->columnNames = std::make_unique>(GetQueryResult().getColumnNames()); } Napi::Value NodeQueryResult::GetQuerySummaryAsync(const Napi::CallbackInfo& info) { @@ -245,7 +262,7 @@ Napi::Value NodeQueryResult::GetQuerySummarySync(const Napi::CallbackInfo& info) Napi::HandleScope scope(env); try { Napi::Object summary = Napi::Object::New(env); - auto cppSummary = this->queryResult->getQuerySummary(); + auto cppSummary = GetQueryResult().getQuerySummary(); summary.Set("compilingTime", Napi::Number::New(env, cppSummary->getCompilingTime())); summary.Set("executionTime", Napi::Number::New(env, cppSummary->getExecutionTime())); return summary; @@ -258,11 +275,15 @@ Napi::Value NodeQueryResult::GetQuerySummarySync(const Napi::CallbackInfo& info) void NodeQueryResult::Close(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); Napi::HandleScope scope(env); - this->Close(); + try { + ThrowIfAsyncOperationInFlight("close"); + this->Close(); + } catch (const std::exception& exc) { + Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); + } } void NodeQueryResult::Close() { columnNames.reset(); ownedQueryResult.reset(); - queryResult = nullptr; } diff --git a/src_js/connection.js b/src_js/connection.js index 575d9f2..3e469d4 100644 --- a/src_js/connection.js +++ b/src_js/connection.js @@ -327,11 +327,13 @@ class Connection { */ _getNextQueryResult(nodeQueryResult) { return new Promise((resolve, reject) => { - const nextNodeQueryResult = new LbugNative.NodeQueryResult(); - nodeQueryResult.getNextQueryResultAsync(nextNodeQueryResult, (err) => { + nodeQueryResult.getNextQueryResultAsync((err, nextNodeQueryResult) => { if (err) { return reject(err); } + if (!nextNodeQueryResult) { + return resolve(undefined); + } return resolve(new QueryResult(this, nextNodeQueryResult)); }); }); @@ -370,8 +372,7 @@ class Connection { const queryResults = [wrappedQueryResult]; let currentQueryResult = nodeQueryResult; while (currentQueryResult.hasNextQueryResult()) { - const nextNodeQueryResult = new LbugNative.NodeQueryResult(); - currentQueryResult.getNextQueryResultSync(nextNodeQueryResult); + const nextNodeQueryResult = currentQueryResult.getNextQueryResultSync(); const nextQueryResult = new QueryResult(this, nextNodeQueryResult); queryResults.push(nextQueryResult); currentQueryResult = nextNodeQueryResult; diff --git a/test/test_connection.js b/test/test_connection.js index b1a51bb..b6444f9 100644 --- a/test/test_connection.js +++ b/test/test_connection.js @@ -180,6 +180,9 @@ describe("Query", function () { childResult.resetIterator(); const secondChildTuple = await childResult.getNext(); assert.equal(secondChildTuple["second"], 2); + childResult.resetIterator(); + const thirdChildTuple = await childResult.getNext(); + assert.equal(thirdChildTuple["second"], 2); childResult.close(); }); @@ -204,6 +207,17 @@ describe("Query", function () { thirdResult.close(); }); + it("should allow repeated close on detached query results", async function () { + const queryResults = await conn.query("RETURN 1 AS first; RETURN 2 AS second;"); + const parentResult = queryResults[0]; + const childResult = queryResults[1]; + + parentResult.close(); + parentResult.close(); + childResult.close(); + childResult.close(); + }); + it("should throw error if one of the multiple queries is invalid", async function () { try { await conn.query(` diff --git a/test/test_sync_api.js b/test/test_sync_api.js index 8fa8991..3032003 100644 --- a/test/test_sync_api.js +++ b/test/test_sync_api.js @@ -39,6 +39,9 @@ describe("Query execution", function () { childResult.resetIterator(); const secondChildTuple = childResult.getNextSync(); assert.equal(secondChildTuple["second"], 2); + childResult.resetIterator(); + const thirdChildTuple = childResult.getNextSync(); + assert.equal(thirdChildTuple["second"], 2); childResult.close(); }); @@ -61,6 +64,17 @@ describe("Query execution", function () { thirdResult.close(); }); + it("should allow repeated close on detached query results", function () { + const queryResults = conn.querySync("RETURN 1 AS first; RETURN 2 AS second;"); + const parentResult = queryResults[0]; + const childResult = queryResults[1]; + + parentResult.close(); + parentResult.close(); + childResult.close(); + childResult.close(); + }); + it("should execute a prepared statement synchronously", function () { const preparedStatement = conn.prepareSync( "RETURN $a as id"