diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e87862..739cf9d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,7 +34,9 @@ jobs: - name: Checkout nodejs_api uses: actions/checkout@v4 with: + fetch-depth: 1 path: tools/nodejs_api + submodules: recursive - name: Fetch ladybug source archive shell: bash @@ -45,13 +47,16 @@ jobs: # it into the working directory, excluding tools/nodejs_api since we # already have it checked out there. - - name: Fetch dataset archive - shell: bash - run: | - mkdir -p dataset - curl -fsSL https://github.com/ladybugdb/dataset/archive/refs/heads/main.tar.gz \ - | tar -xz --strip-components=1 -C dataset - # Tests reference ../../dataset/ relative to tools/nodejs_api. + - name: Setup ccache (non-Windows) + if: ${{ matrix.platform != 'win32' }} + uses: hendrikmuhs/ccache-action@v1.2 + with: + key: nodejs-${{ runner.os }}-${{ runner.arch }}-${{ github.ref }} + max-size: 2G + create-symlink: true + restore-keys: | + nodejs-${{ runner.os }}-${{ runner.arch }}-refs/heads/main + nodejs-${{ runner.os }}-${{ runner.arch }}- - name: Install Node.js dependencies (non-Windows) if: ${{ matrix.platform != 'win32' }} @@ -117,6 +122,8 @@ jobs: env: MACOSX_DEPLOYMENT_TARGET: ${{ matrix.mac_env && '13.3' || '' }} CMAKE_OSX_ARCHITECTURES: ${{ matrix.mac_env && matrix.arch || '' }} + CMAKE_C_COMPILER_LAUNCHER: ccache + CMAKE_CXX_COMPILER_LAUNCHER: ccache - name: Run Node.js tests working-directory: tools/nodejs_api diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 0000000..2638e6f --- /dev/null +++ b/.gitmodules @@ -0,0 +1,3 @@ +[submodule "dataset"] + path = dataset + url = https://github.com/ladybugdb/dataset.git diff --git a/CMakeLists.txt b/CMakeLists.txt index 42de264..7fdffe3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,18 @@ +cmake_minimum_required(VERSION 3.15) +project(lbugjs LANGUAGES CXX C) + +set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CMAKE_CXX_EXTENSIONS OFF) + add_definitions(-DNAPI_VERSION=6) add_definitions(-DNODE_RUNTIME=node) add_definitions(-DBUILDING_NODE_EXTENSION) +set(LBUG_SOURCE_DIR "" CACHE PATH "Path to the Ladybug source tree used for standalone Node.js builds") +set(LBUG_BUILD_DIR "" CACHE PATH "Path to the Ladybug build tree used for standalone Node.js builds") +set(LBUG_NODEJS_EXTRA_LINK_LIBS "" CACHE STRING "Additional link libraries for standalone Node.js builds against a precompiled liblbug") + # If on Windows use npx.cmd instead of npx if(WIN32) set(NPX_CMD npx.cmd) @@ -27,6 +38,12 @@ execute_process( OUTPUT_VARIABLE CMAKE_JS_SRC ERROR_QUIET ) +execute_process( + COMMAND node -p "process.config.variables.node_prefix || ''" + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} + OUTPUT_VARIABLE NODE_PREFIX + ERROR_QUIET +) # Without this windows breaks with: Invalid character escape '\P'. string(REPLACE "\\" "/" CMAKE_JS_INC "${CMAKE_JS_INC}") @@ -46,6 +63,7 @@ string(REPLACE "\\" "/" CMAKE_JS_SRC "${CMAKE_JS_SRC}") string(STRIP "${CMAKE_JS_INC}" CMAKE_JS_INC) string(STRIP "${CMAKE_JS_LIB}" CMAKE_JS_LIB) string(STRIP "${CMAKE_JS_SRC}" CMAKE_JS_SRC) +string(STRIP "${NODE_PREFIX}" NODE_PREFIX) # Extract last line: remove everything up to and including the last newline. string(REGEX REPLACE ".*\n" "" CMAKE_JS_INC "${CMAKE_JS_INC}") string(REGEX REPLACE ".*\n" "" CMAKE_JS_LIB "${CMAKE_JS_LIB}") @@ -61,6 +79,12 @@ endif() if(NOT CMAKE_JS_SRC MATCHES "^(/|[A-Za-z]:)") set(CMAKE_JS_SRC "") endif() +if(NOT CMAKE_JS_INC AND NODE_PREFIX MATCHES "^(/|[A-Za-z]:)") + set(CMAKE_JS_INC "${NODE_PREFIX}/include/node") +endif() +if(WIN32 AND NOT CMAKE_JS_LIB AND NODE_PREFIX MATCHES "^(/|[A-Za-z]:)") + set(CMAKE_JS_LIB "${NODE_PREFIX}/lib/node.lib") +endif() # Print CMAKE_JS variables message(STATUS "CMake.js configurations: LIB=${CMAKE_JS_LIB}, INC=${CMAKE_JS_INC}, SRC=${CMAKE_JS_SRC}") @@ -90,15 +114,29 @@ if(NOT TARGET lbug) if(NOT EXISTS "${LBUG_NODEJS_PRECOMPILED_LIB_PATH}") message(FATAL_ERROR "Precompiled liblbug archive not found: ${LBUG_NODEJS_PRECOMPILED_LIB_PATH}") endif() + + set(LBUG_NODEJS_SOURCE_INCLUDE_DIR "${PROJECT_SOURCE_DIR}/src/include") + set(LBUG_NODEJS_BUILD_INCLUDE_DIR "${PROJECT_BINARY_DIR}/src/include") + if(LBUG_SOURCE_DIR) + set(LBUG_NODEJS_SOURCE_INCLUDE_DIR "${LBUG_SOURCE_DIR}/src/include") + endif() + if(LBUG_BUILD_DIR) + set(LBUG_NODEJS_BUILD_INCLUDE_DIR "${LBUG_BUILD_DIR}/src/include") + elseif(LBUG_SOURCE_DIR) + set(LBUG_NODEJS_BUILD_INCLUDE_DIR "${LBUG_SOURCE_DIR}/build/release/src/include") + endif() + add_library(lbug STATIC IMPORTED GLOBAL) set_target_properties(lbug PROPERTIES IMPORTED_LOCATION "${LBUG_NODEJS_PRECOMPILED_LIB_PATH}") target_include_directories(lbug INTERFACE - "${PROJECT_SOURCE_DIR}/src/include" - "${PROJECT_BINARY_DIR}/src/include") + "${LBUG_NODEJS_SOURCE_INCLUDE_DIR}" + "${LBUG_NODEJS_BUILD_INCLUDE_DIR}") if(WIN32) target_compile_definitions(lbug INTERFACE LBUG_STATIC_DEFINE) endif() - set(NODEJS_LBUG_LINK_DEPS "$") + if(TARGET lbug_link_deps) + set(NODEJS_LBUG_LINK_DEPS "$") + endif() endif() add_library(lbugjs SHARED ${CPP_SOURCE_FILES}) @@ -117,3 +155,6 @@ else() target_link_options(lbugjs PRIVATE -Wl,--export-dynamic) endif() target_link_libraries(lbugjs PRIVATE lbug ${NODEJS_LBUG_LINK_DEPS} ${CMAKE_JS_LIB}) +if(LBUG_NODEJS_EXTRA_LINK_LIBS) + target_link_libraries(lbugjs PRIVATE ${LBUG_NODEJS_EXTRA_LINK_LIBS}) +endif() diff --git a/README.md b/README.md index 086c63e..42b8963 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,7 @@ Both CommonJS (`require`) and ES Modules (`import`) are fully supported. ### Install Dev Dependencies ```bash +git submodule update --init --recursive npm install --include=dev ``` @@ -90,6 +91,14 @@ npm install --include=dev npm run build ``` +By default, the standalone build looks for a sibling Ladybug checkout at +`../ladybug` via `LBUG_SOURCE_DIR`, and reuses the prebuilt static library from +`../ladybug/build/release`. Override that location like this: + +```bash +LBUG_SOURCE_DIR=/path/to/ladybug npm run build +``` + ### Run Tests ```bash diff --git a/build.js b/build.js index 2c7c29f..71f30b6 100644 --- a/build.js +++ b/build.js @@ -1,29 +1,146 @@ const fs = require("fs"); +const os = require("os"); const path = require("path"); -const { execSync } = require("child_process"); +const { execFileSync, execSync } = require("child_process"); -const SRC_PATH = path.resolve(__dirname, "../.."); -const THREADS = require("os").cpus().length; +const THREADS = os.cpus().length; +const DEFAULT_LBUG_SOURCE_DIR = path.resolve(__dirname, "../ladybug"); + +function resolveExistingPath(candidate) { + if (!candidate) { + return null; + } + const resolved = path.resolve(candidate); + return fs.existsSync(resolved) ? resolved : null; +} + +function getDefaultBuildDir(lbugSourceDir) { + return lbugSourceDir ? path.join(lbugSourceDir, "build", "release") : null; +} + +function getDefaultPrecompiledLibPath(lbugBuildDir) { + if (!lbugBuildDir) { + return null; + } + const candidates = process.platform === "win32" + ? [ + path.join(lbugBuildDir, "src", "Release", "lbug.lib"), + path.join(lbugBuildDir, "src", "lbug.lib"), + path.join(lbugBuildDir, "src", "Release", "liblbug.lib"), + path.join(lbugBuildDir, "src", "liblbug.lib"), + ] + : [path.join(lbugBuildDir, "src", "liblbug.a")]; + return candidates.find((candidate) => fs.existsSync(candidate)) || null; +} + +function getDefaultExtraLinkLibs(lbugBuildDir, precompiledLibPath) { + if (!lbugBuildDir || !precompiledLibPath) { + return []; + } + + const linkTxtCandidates = [ + path.join(lbugBuildDir, "tools", "python_api", "CMakeFiles", "_lbug.dir", "link.txt"), + path.join(lbugBuildDir, "src", "CMakeFiles", "lbug_shared.dir", "link.txt"), + ]; + const tokenPattern = /"[^"]+"|\S+/g; + + for (const linkTxtPath of linkTxtCandidates) { + if (!fs.existsSync(linkTxtPath)) { + continue; + } + const linkTxtDir = path.dirname(linkTxtPath); + const linkBaseDir = linkTxtPath.includes(`${path.sep}tools${path.sep}python_api${path.sep}`) + ? path.join(lbugBuildDir, "tools", "python_api") + : linkTxtPath.includes(`${path.sep}src${path.sep}CMakeFiles${path.sep}`) + ? path.join(lbugBuildDir, "src") + : linkTxtDir; + const tokens = fs.readFileSync(linkTxtPath, "utf8").match(tokenPattern) || []; + const libs = []; + let sawPrecompiledLib = false; + + for (const rawToken of tokens) { + const token = rawToken.replace(/^"(.*)"$/, "$1"); + const resolvedToken = token.startsWith("-") + ? token + : fs.existsSync(path.resolve(linkBaseDir, token)) + ? path.resolve(linkBaseDir, token) + : path.resolve(linkTxtDir, token); + + if (!sawPrecompiledLib) { + if (resolvedToken === precompiledLibPath) { + sawPrecompiledLib = true; + } + continue; + } + + if (token.startsWith("-l")) { + libs.push(token); + continue; + } + if (!/\.(a|lib|dylib|so|tbd)$/.test(token)) { + continue; + } + if (resolvedToken === precompiledLibPath) { + continue; + } + libs.push(resolvedToken); + } + + if (libs.length > 0) { + return [...new Set(libs)]; + } + } + + return []; +} console.log(`Using ${THREADS} threads to build Lbug.`); const env = { ...process.env }; -const precompiledLibPath = env.LBUG_NODEJS_PRECOMPILED_LIB_PATH; +const lbugSourceDir = resolveExistingPath(env.LBUG_SOURCE_DIR || DEFAULT_LBUG_SOURCE_DIR); +const lbugBuildDir = resolveExistingPath(env.LBUG_BUILD_DIR || getDefaultBuildDir(lbugSourceDir)); +const precompiledLibPath = resolveExistingPath( + env.LBUG_NODEJS_PRECOMPILED_LIB_PATH || getDefaultPrecompiledLibPath(lbugBuildDir) +); +const extraLinkLibs = getDefaultExtraLinkLibs(lbugBuildDir, precompiledLibPath); + +if (lbugSourceDir) { + env.LBUG_SOURCE_DIR = lbugSourceDir; +} +if (lbugBuildDir) { + env.LBUG_BUILD_DIR = lbugBuildDir; +} + +const cmakeArgs = env.EXTRA_CMAKE_FLAGS + ? env.EXTRA_CMAKE_FLAGS.trim().split(/\s+/).filter(Boolean) + : []; +if (lbugSourceDir) { + cmakeArgs.push(`-DLBUG_SOURCE_DIR=${lbugSourceDir}`); +} +if (lbugBuildDir) { + cmakeArgs.push(`-DLBUG_BUILD_DIR=${lbugBuildDir}`); +} if (precompiledLibPath) { - const extraFlags = [ - env.EXTRA_CMAKE_FLAGS, + cmakeArgs.push( "-DBUILD_LBUG=FALSE", "-DBUILD_SHELL=FALSE", "-DLBUG_NODEJS_USE_PRECOMPILED_LIB=TRUE", - `-DLBUG_NODEJS_PRECOMPILED_LIB_PATH=${precompiledLibPath}`, - ].filter(Boolean).join(" "); - env.EXTRA_CMAKE_FLAGS = extraFlags; + `-DLBUG_NODEJS_PRECOMPILED_LIB_PATH=${precompiledLibPath}` + ); console.log(`Using precompiled liblbug from ${precompiledLibPath}.`); } +if (extraLinkLibs.length > 0) { + cmakeArgs.push(`-DLBUG_NODEJS_EXTRA_LINK_LIBS=${extraLinkLibs.join(";")}`); +} execSync("npm run clean", { stdio: "inherit" }); -execSync(`make nodejs NUM_THREADS=${THREADS}`, { - cwd: SRC_PATH, +execFileSync("cmake", ["-S", ".", "-B", "build", ...cmakeArgs], { + cwd: __dirname, + env, + stdio: "inherit", +}); +execFileSync("cmake", ["--build", "build", "-j", `${THREADS}`], { + cwd: __dirname, env, stdio: "inherit", }); diff --git a/dataset b/dataset new file mode 160000 index 0000000..5553111 --- /dev/null +++ b/dataset @@ -0,0 +1 @@ +Subproject commit 55531118c5e0c683fc3a3d806b7abd0b09a31ff8 diff --git a/install.js b/install.js index 358239f..cc11652 100644 --- a/install.js +++ b/install.js @@ -7,6 +7,34 @@ const process = require("process"); const isNpmBuildFromSourceSet = process.env.npm_config_build_from_source; const platform = process.platform; const arch = process.arch; +const DEFAULT_LBUG_SOURCE_DIR = path.resolve(__dirname, "../ladybug"); + +function resolveExistingPath(candidate) { + if (!candidate) { + return null; + } + const resolved = path.resolve(candidate); + return fs.existsSync(resolved) ? resolved : null; +} + +function getDefaultBuildDir(lbugSourceDir) { + return lbugSourceDir ? path.join(lbugSourceDir, "build", "release") : null; +} + +function getDefaultPrecompiledLibPath(lbugBuildDir) { + if (!lbugBuildDir) { + return null; + } + const candidates = platform === "win32" + ? [ + path.join(lbugBuildDir, "src", "Release", "lbug.lib"), + path.join(lbugBuildDir, "src", "lbug.lib"), + path.join(lbugBuildDir, "src", "Release", "liblbug.lib"), + path.join(lbugBuildDir, "src", "liblbug.lib"), + ] + : [path.join(lbugBuildDir, "src", "liblbug.a")]; + return candidates.find((candidate) => fs.existsSync(candidate)) || null; +} // When the package was published with prebuilt binaries, each platform's // binary lives in a dedicated optional sub-package. npm may hoist it to the @@ -59,6 +87,17 @@ if (isNpmBuildFromSourceSet) { console.log("Prebuilt binary is not available, building from source..."); } +const externalLbugSourceDir = resolveExistingPath( + process.env.LBUG_SOURCE_DIR || DEFAULT_LBUG_SOURCE_DIR +); +const externalLbugBuildDir = resolveExistingPath( + process.env.LBUG_BUILD_DIR || getDefaultBuildDir(externalLbugSourceDir) +); +const externalPrecompiledLibPath = resolveExistingPath( + process.env.LBUG_NODEJS_PRECOMPILED_LIB_PATH || + getDefaultPrecompiledLibPath(externalLbugBuildDir) +); + // Get number of threads const THREADS = os.cpus().length; console.log(`Using ${THREADS} threads to build Lbug.`); @@ -73,6 +112,12 @@ childProcess.execSync("npm install", { // Build the Lbug source code console.log("Building Lbug source code..."); const env = { ...process.env }; +if (externalLbugSourceDir) { + env.LBUG_SOURCE_DIR = externalLbugSourceDir; +} +if (externalLbugBuildDir) { + env.LBUG_BUILD_DIR = externalLbugBuildDir; +} if (process.platform === "darwin") { const archflags = process.env["ARCHFLAGS"] @@ -128,16 +173,19 @@ if (process.platform === "win32") { ); } -if (env.LBUG_NODEJS_PRECOMPILED_LIB_PATH) { +if (externalPrecompiledLibPath) { + env.LBUG_NODEJS_PRECOMPILED_LIB_PATH = externalPrecompiledLibPath; env.EXTRA_CMAKE_FLAGS = [ env.EXTRA_CMAKE_FLAGS, + env.LBUG_SOURCE_DIR ? `-DLBUG_SOURCE_DIR=${env.LBUG_SOURCE_DIR}` : null, + env.LBUG_BUILD_DIR ? `-DLBUG_BUILD_DIR=${env.LBUG_BUILD_DIR}` : null, "-DBUILD_LBUG=FALSE", "-DBUILD_SHELL=FALSE", "-DLBUG_NODEJS_USE_PRECOMPILED_LIB=TRUE", - `-DLBUG_NODEJS_PRECOMPILED_LIB_PATH=${env.LBUG_NODEJS_PRECOMPILED_LIB_PATH}`, + `-DLBUG_NODEJS_PRECOMPILED_LIB_PATH=${externalPrecompiledLibPath}`, ].filter(Boolean).join(" "); console.log( - `Using precompiled liblbug from '${env.LBUG_NODEJS_PRECOMPILED_LIB_PATH}'.` + `Using precompiled liblbug from '${externalPrecompiledLibPath}'.` ); } diff --git a/src_cpp/include/node_connection.h b/src_cpp/include/node_connection.h index a47be64..7406a40 100644 --- a/src_cpp/include/node_connection.h +++ b/src_cpp/include/node_connection.h @@ -105,7 +105,7 @@ class ConnectionExecuteAsyncWorker : public Napi::AsyncWorker { SetError(result->getErrorMessage()); return; } - nodeQueryResult->AdoptQueryResult(std::move(result), database); + nodeQueryResult->AdoptQueryResult(std::move(result), connection, database); } catch (const std::exception& exc) { SetError(std::string(exc.what())); } @@ -132,8 +132,8 @@ class ConnectionExecuteAsyncWorker : public Napi::AsyncWorker { class ConnectionQueryAsyncWorker : public Napi::AsyncWorker { public: ConnectionQueryAsyncWorker(Napi::Function& callback, std::shared_ptr& connection, - std::shared_ptr& database, - std::string statement, NodeQueryResult* nodeQueryResult, Napi::Value progressCallback) + std::shared_ptr& database, std::string statement, + NodeQueryResult* nodeQueryResult, Napi::Value progressCallback) : Napi::AsyncWorker(callback), connection(connection), database(database), statement(std::move(statement)), nodeQueryResult(nodeQueryResult) { if (progressCallback.IsFunction()) { @@ -162,7 +162,7 @@ class ConnectionQueryAsyncWorker : public Napi::AsyncWorker { SetError(result->getErrorMessage()); return; } - nodeQueryResult->AdoptQueryResult(std::move(result), database); + nodeQueryResult->AdoptQueryResult(std::move(result), connection, database); } 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 07d1345..b300eb7 100644 --- a/src_cpp/include/node_query_result.h +++ b/src_cpp/include/node_query_result.h @@ -21,9 +21,10 @@ 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, - std::shared_ptr db); + std::shared_ptr connection, std::shared_ptr database); explicit NodeQueryResult(const Napi::CallbackInfo& info); - void AdoptQueryResult(std::unique_ptr queryResult, std::shared_ptr db); + void AdoptQueryResult(std::unique_ptr queryResult, + std::shared_ptr connection, std::shared_ptr database); std::unique_ptr DetachNextQueryResult(); ~NodeQueryResult() override; @@ -53,6 +54,7 @@ class NodeQueryResult : public Napi::ObjectWrap { private: static Napi::FunctionReference constructor; std::unique_ptr ownedQueryResult = nullptr; + std::shared_ptr connection = nullptr; std::shared_ptr database = nullptr; std::unique_ptr> columnNames = nullptr; std::atomic activeAsyncUses = 0; @@ -205,7 +207,7 @@ class NodeQueryResultGetNextQueryResultAsyncWorker : public Napi::AsyncWorker { return; } Callback().Call({env.Null(), NodeQueryResult::NewInstance(env, std::move(nextOwnedResult), - currQueryResult->database)}); + currQueryResult->connection, currQueryResult->database)}); } void OnError(Napi::Error const& error) override { diff --git a/src_cpp/node_connection.cpp b/src_cpp/node_connection.cpp index c093fbf..8857666 100644 --- a/src_cpp/node_connection.cpp +++ b/src_cpp/node_connection.cpp @@ -116,7 +116,7 @@ Napi::Value NodeConnection::QuerySync(const Napi::CallbackInfo& info) { if (!result->isSuccess()) { Napi::Error::New(env, result->getErrorMessage()).ThrowAsJavaScriptException(); } - nodeQueryResult->AdoptQueryResult(std::move(result), database); + nodeQueryResult->AdoptQueryResult(std::move(result), connection, database); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -136,7 +136,7 @@ Napi::Value NodeConnection::ExecuteSync(const Napi::CallbackInfo& info) { if (!result->isSuccess()) { Napi::Error::New(env, result->getErrorMessage()).ThrowAsJavaScriptException(); } - nodeQueryResult->AdoptQueryResult(std::move(result), database); + nodeQueryResult->AdoptQueryResult(std::move(result), connection, database); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -150,7 +150,8 @@ Napi::Value NodeConnection::QueryAsync(const Napi::CallbackInfo& info) { auto nodeQueryResult = Napi::ObjectWrap::Unwrap(info[1].As()); auto callback = info[2].As(); auto asyncWorker = - new ConnectionQueryAsyncWorker(callback, connection, database, statement, nodeQueryResult, info[3]); + new ConnectionQueryAsyncWorker( + callback, connection, database, statement, nodeQueryResult, info[3]); asyncWorker->Queue(); return info.Env().Undefined(); } diff --git a/src_cpp/node_query_result.cpp b/src_cpp/node_query_result.cpp index 2d8f854..931b5c5 100644 --- a/src_cpp/node_query_result.cpp +++ b/src_cpp/node_query_result.cpp @@ -37,10 +37,12 @@ Napi::Object NodeQueryResult::Init(Napi::Env env, Napi::Object exports) { } Napi::Object NodeQueryResult::NewInstance( - Napi::Env /*env*/, std::unique_ptr queryResult, std::shared_ptr db) { + Napi::Env /*env*/, std::unique_ptr queryResult, + std::shared_ptr connection, std::shared_ptr database) { auto obj = constructor.New({}); auto* nodeQueryResult = Napi::ObjectWrap::Unwrap(obj); - nodeQueryResult->AdoptQueryResult(std::move(queryResult), std::move(db)); + nodeQueryResult->AdoptQueryResult( + std::move(queryResult), std::move(connection), std::move(database)); return obj; } @@ -52,11 +54,13 @@ NodeQueryResult::~NodeQueryResult() { } void NodeQueryResult::AdoptQueryResult( - std::unique_ptr queryResult, std::shared_ptr db) { + std::unique_ptr queryResult, std::shared_ptr connection, + std::shared_ptr database) { ThrowIfAsyncOperationInFlight("replace"); columnNames.reset(); ownedQueryResult = std::move(queryResult); - database = std::move(db); + this->connection = std::move(connection); + this->database = std::move(database); } std::unique_ptr NodeQueryResult::DetachNextQueryResult() { @@ -142,7 +146,7 @@ Napi::Value NodeQueryResult::GetNextQueryResultSync(const Napi::CallbackInfo& in .ThrowAsJavaScriptException(); return env.Undefined(); } - return NewInstance(env, std::move(nextOwnedResult), database); + return NewInstance(env, std::move(nextOwnedResult), connection, database); } catch (const std::exception& exc) { Napi::Error::New(env, std::string(exc.what())).ThrowAsJavaScriptException(); } @@ -288,5 +292,6 @@ void NodeQueryResult::Close(const Napi::CallbackInfo& info) { void NodeQueryResult::Close() { columnNames.reset(); ownedQueryResult.reset(); + connection.reset(); database.reset(); } diff --git a/test/common.js b/test/common.js index 433ca94..edc4c9e 100644 --- a/test/common.js +++ b/test/common.js @@ -18,6 +18,9 @@ if (TEST_INSTALLED) { const tmp = require("tmp"); const fs = require("fs/promises"); const path = require("path"); + +const toCypherPath = (filePath) => filePath.replace(/\\/g, "/"); + const initTests = async () => { const tmpPath = await new Promise((resolve, reject) => { tmp.dir({ unsafeCleanup: true }, (err, path, _) => { @@ -31,7 +34,11 @@ const initTests = async () => { const dbPath = path.join(tmpPath, "db.lbdb"); const db = new lbug.Database(dbPath, 1 << 28 /* 256MB */); const conn = new lbug.Connection(db, 4); - const tinysnbDir = "../../dataset/tinysnb/"; + const datasetDir = path.resolve(__dirname, "..", "dataset"); + const tinysnbDir = path.join(datasetDir, "tinysnb") + path.sep; + const tinysnbSerialDir = path.join(datasetDir, "tinysnb-serial") + path.sep; + const cypherTinysnbDir = toCypherPath(tinysnbDir); + const cypherTinysnbSerialDir = toCypherPath(tinysnbSerialDir); const schema = (await fs.readFile(tinysnbDir + "schema.cypher")) .toString() @@ -56,7 +63,7 @@ const initTests = async () => { } // handle multiple data files in one line - const statement = line.replace(dataFileRegex, `"${tinysnbDir}$1"`); + const statement = line.replace(dataFileRegex, `"${cypherTinysnbDir}$1"`); await conn.query(statement); } @@ -65,7 +72,7 @@ const initTests = async () => { "create node table moviesSerial (ID SERIAL, name STRING, length INT32, note STRING, PRIMARY KEY (ID))" ); await conn.query( - 'copy moviesSerial from "../../dataset/tinysnb-serial/vMovies.csv"' + `copy moviesSerial from "${cypherTinysnbSerialDir}vMovies.csv"` ); global.dbPath = dbPath; diff --git a/test/test_database.js b/test/test_database.js index 916ba84..4717d2b 100644 --- a/test/test_database.js +++ b/test/test_database.js @@ -518,8 +518,9 @@ describe("Database close", function () { // MaterializedQueryResult whose FactorizedTable destructor accesses // database-owned memory. If the Database is destroyed before the GC // finalizer for NodeQueryResult runs, that destructor crashes. The fix - // is for NodeQueryResult to hold a shared_ptr so the Database - // cannot be freed until every result that references it is gone. + // is for NodeQueryResult to hold a shared_ptr so the native + // connection keeps the database-backed state alive until every result + // that references it is gone. // // The key pattern being tested is: query results are *not stored*, making // them immediately eligible for GC. conn.closeSync() and db.closeSync() @@ -536,4 +537,26 @@ describe("Database close", function () { assert.isTrue(conn._isClosed); assert.isTrue(testDb._isClosed); }); + + it("should not crash when file-backed async query results are discarded before close", async function () { + const tmpDbPath = await new Promise((resolve, reject) => { + tmp.dir({ unsafeCleanup: true }, (err, path, _) => { + if (err) { + return reject(err); + } + return resolve(path); + }); + }); + const dbPath = path.join(tmpDbPath, "test.lbdb"); + const testDb = new lbug.Database(dbPath); + const conn = new lbug.Connection(testDb); + await conn.query("CREATE NODE TABLE IF NOT EXISTS T(id STRING PRIMARY KEY)"); + const id = `test-${Date.now()}`; + await conn.query(`CREATE (:T {id: '${id}'})`); + await conn.query("MATCH (t:T) RETURN t.id"); + conn.closeSync(); + testDb.closeSync(); + assert.isTrue(conn._isClosed); + assert.isTrue(testDb._isClosed); + }); });