From d045f4c368d2a61ce0b5f5c25b93fa1ac6b794ae Mon Sep 17 00:00:00 2001 From: Sharad Chandran R Date: Fri, 22 Sep 2023 16:15:41 +0530 Subject: [PATCH] Code refactoring + bug fixes for cursor leaks and SODA lock setting --- doc/src/release_notes.rst | 3 + lib/thin/connection.js | 255 ++++++++++++------------- lib/thin/protocol/messages/withData.js | 2 +- lib/thin/util.js | 90 ++------- src/njsSodaOperation.c | 4 + 5 files changed, 146 insertions(+), 208 deletions(-) diff --git a/doc/src/release_notes.rst b/doc/src/release_notes.rst index 6a2fd22d9..59b868385 100644 --- a/doc/src/release_notes.rst +++ b/doc/src/release_notes.rst @@ -25,6 +25,9 @@ Thin Mode Changes in embedded quotes and in JSON syntax. `Issue #1605 `__. +#) Corrected bug that caused cursors to be leaked when calling + Connection.getStatementInfo(). + #) Fixed bug that caused an exception to be thrown unnecessarily when a connection was closed. `Issue #1604 `__. diff --git a/lib/thin/connection.js b/lib/thin/connection.js index c7a6b6311..aac81f637 100644 --- a/lib/thin/connection.js +++ b/lib/thin/connection.js @@ -159,6 +159,98 @@ class ThinConnectionImpl extends ConnectionImpl { } } + //--------------------------------------------------------------------------- + // _execute() + // + // Calls the RPC that executes a SQL statement and returns the results. + //--------------------------------------------------------------------------- + async _execute(statement, numIters, binds, options, executeManyFlag) { + + // perform binds + const numBinds = statement.bindInfoList.length; + const numVars = binds.length; + if (numBinds !== numVars && (numVars === 0 || !binds[0].name)) { + errors.throwErr(errors.ERR_WRONG_NUMBER_OF_POSITIONAL_BINDS, numBinds, numVars); + } + for (let i = 0; i < binds.length; i++) { + await this._bind(statement, binds[i], i + 1); + } + if (statement.isPlSql && (options.batchErrors || options.dmlRowCounts)) { + errors.throwErr(errors.ERR_EXEC_MODE_ONLY_FOR_DML); + } + + // send database request + const message = new messages.ExecuteMessage(this, statement, options); + message.numExecs = numIters; + message.arrayDmlRowCounts = options.dmlRowCounts; + message.batchErrors = options.batchErrors; + if (statement.isPlSql && statement.requiresFullExecute) { + message.numExecs = 1; + await this._protocol._processMessage(message); + statement.requiresFullExecute = false; + message.numExecs = numIters - 1; + message.offset = 1; + } + if (message.numExecs > 0) { + await this._protocol._processMessage(message); + statement.requiresFullExecute = false; + } + + // if a define is required, send an additional request to the database + if (statement.requiresDefine && statement.sql) { + statement.requiresFullExecute = true; + await this._protocol._processMessage(message); + statement.requiresFullExecute = false; + statement.requiresDefine = false; + } + + // process results + const result = {}; + if (statement.numQueryVars > 0) { + result.resultSet = message.resultSet; + } else { + statement.bufferRowIndex = 0; + const outBinds = thinUtil.getOutBinds(statement, numIters, + executeManyFlag); + if (outBinds) { + result.outBinds = outBinds; + } + if (executeManyFlag) { + if (!statement.isPlSql) { + result.rowsAffected = statement.rowCount; + delete statement.rowCount; + } + if (options.dmlRowCounts) { + result.dmlRowCounts = options.dmlRowCounts; + } + if (options.batchErrors) { + result.batchErrors = options.batchErrors; + } + } else { + if (statement.isPlSql && options.implicitResultSet) { + result.implicitResults = options.implicitResultSet; + } + if (statement.lastRowid) { + result.lastRowid = statement.lastRowid; + delete statement.lastRowid; + } + if (statement.isPlSql) { + if (statement.rowCount) { + result.rowsAffected = statement.rowCount; + } + } else { + result.rowsAffected = statement.rowCount || 0; + } + if (statement.rowCount) { + delete statement.rowCount; + } + } + this._returnStatement(statement); + } + + return result; + } + //--------------------------------------------------------------------------- // _parseElementType() // @@ -778,26 +870,6 @@ class ThinConnectionImpl extends ConnectionImpl { return resultSet; } - //--------------------------------------------------------------------------- - // Prepares the sql given by user and binds the value to the statement object - //--------------------------------------------------------------------------- - async _prepareAndBind(sql, binds, options, isParse = false) { - const stmt = this._prepare(sql, options); - let position = 0; - if (!isParse) { - const numBinds = stmt.bindInfoList.length; - const numVars = binds.length; - if (numBinds !== numVars && (numVars === 0 || !binds[0].name)) { - errors.throwErr(errors.ERR_WRONG_NUMBER_OF_POSITIONAL_BINDS, numBinds, numVars); - } - for (const variable of binds) { - await this._bind(stmt, variable, position + 1); - position += 1; - } - } - return stmt; - } - //--------------------------------------------------------------------------- // Clears the statement cache for the connection //--------------------------------------------------------------------------- @@ -818,139 +890,46 @@ class ThinConnectionImpl extends ConnectionImpl { } //--------------------------------------------------------------------------- - // Parses the sql given by User - // calls the OAL8 RPC that parses the SQL statement and returns the metadata - // information for a statment. + // getStatementInfo() + // + // Parses the SQL statement and returns information about the statement. //--------------------------------------------------------------------------- async getStatementInfo(sql) { const options = {}; const result = {}; - const statement = await this._prepareAndBind(sql, null, options, true); + const statement = this._prepare(sql, options); options.connection = this; - // parse the statement (but not for DDL which doesn't support it) - if (!statement.isDdl) { - const message = new messages.ExecuteMessage(this, statement, options); - message.parseOnly = true; - await this._protocol._processMessage(message); - } - if (statement.numQueryVars > 0) { - result.metaData = thinUtil.getMetadataMany(statement.queryVars); - } - result.bindNames = Array.from(statement.bindInfoDict.keys()); - result.statementType = statement.statementType; - return result; - } - - //--------------------------------------------------------------------------- - // Prepares the sql given by the user, - // calls the OAL8 RPC that executes a SQL statement and returns the results. - //--------------------------------------------------------------------------- - async execute(sql, numIters, binds, options, executeManyFlag) { - const result = {}; - if (executeManyFlag) { - return await this.executeMany(sql, numIters, binds, options); - } - const statement = await this._prepareAndBind(sql, binds, options); - - // send the initial request to the database - const message = new messages.ExecuteMessage(this, statement, options); - message.numExecs = 1; - await this._protocol._processMessage(message); - statement.requiresFullExecute = false; - - // if a define is required, send an additional request to the database - if (statement.requiresDefine && statement.sql) { - statement.requiresFullExecute = true; - await this._protocol._processMessage(message); - statement.requiresFullExecute = false; - statement.requiresDefine = false; - } - - // process message results - if (statement.numQueryVars > 0) { - result.resultSet = message.resultSet; - } else { - statement.bufferRowIndex = 0; - const bindVars = thinUtil.getBindVars(statement); - const outBinds = thinUtil.getExecuteOutBinds(bindVars); - if (outBinds) { - result.outBinds = outBinds; - } - if (statement.isPlSql) { - if (options.implicitResultSet) { - result.implicitResults = options.implicitResultSet; - } - } - if (statement.lastRowid) { - result.lastRowid = statement.lastRowid; - delete statement.lastRowid; - } - if (statement.isPlSql) { - if (statement.rowCount) { - result.rowsAffected = statement.rowCount; - } - } else { - result.rowsAffected = statement.rowCount || 0; + try { + if (!statement.isDdl) { + const message = new messages.ExecuteMessage(this, statement, options); + message.parseOnly = true; + await this._protocol._processMessage(message); } - if (statement.rowCount) { - delete statement.rowCount; + if (statement.numQueryVars > 0) { + result.metaData = thinUtil.getMetadataMany(statement.queryVars); } + result.bindNames = Array.from(statement.bindInfoDict.keys()); + result.statementType = statement.statementType; + return result; + } finally { this._returnStatement(statement); } - - return result; } //--------------------------------------------------------------------------- - // executeMany() + // execute() // - // Prepares the sql given by the user, calls the OAL8 RPC that executes a SQL - // statement multiple times and returns the results. + // Calls the RPC that executes a SQL statement and returns the results. //--------------------------------------------------------------------------- - async executeMany(sql, numIters, binds, options) { - const statement = await this._prepareAndBind(sql, binds, options); - if (statement.isPlSql && (options.batchErrors || options.dmlRowCounts)) { - errors.throwErr(errors.ERR_EXEC_MODE_ONLY_FOR_DML); - } - - // send database request - const message = new messages.ExecuteMessage(this, statement, options); - message.numExecs = numIters; - message.arrayDmlRowCounts = options.dmlRowCounts; - message.batchErrors = options.batchErrors; - if (statement.isPlSql && statement.requiresFullExecute) { - message.numExecs = 1; - await this._protocol._processMessage(message); - statement.requiresFullExecute = false; - message.numExecs = numIters - 1; - message.offset = 1; - } - if (message.numExecs > 0) { - await this._protocol._processMessage(message); - } - - // process results - const returnObj = {}; - statement.bufferRowIndex = 0; - const bindVars = thinUtil.getBindVars(statement); - const outBinds = thinUtil.getExecuteManyOutBinds(bindVars, numIters); - if (outBinds) { - returnObj.outBinds = outBinds; - } - const rowsAffected = !statement.isPlSql ? statement.rowCount : undefined; - if (rowsAffected) { - returnObj.rowsAffected = rowsAffected; - delete statement.rowCount; - } - if (options.dmlRowCounts) { - returnObj.dmlRowCounts = options.dmlRowCounts; - } - if (options.batchErrors) { - returnObj.batchErrors = options.batchErrors; + async execute(sql, numIters, binds, options, executeManyFlag) { + const statement = this._prepare(sql, options); + try { + return await this._execute(statement, numIters, binds, options, + executeManyFlag); + } catch (err) { + this._returnStatement(statement); + throw err; } - this._returnStatement(statement); - - return returnObj; } //--------------------------------------------------------------------------- diff --git a/lib/thin/protocol/messages/withData.js b/lib/thin/protocol/messages/withData.js index 439006489..5bd22cc96 100644 --- a/lib/thin/protocol/messages/withData.js +++ b/lib/thin/protocol/messages/withData.js @@ -116,8 +116,8 @@ class MessageWithData extends Message { this.errorOccurred = false; this.statement.moreRowsToFetch = false; } else if (this.errorInfo.num !== 0 && this.errorInfo.cursorId !== 0) { - this.connection._addCursorToClose(this.statement.cursorId); this.connection.statementCache.delete(this.statement.sql); + this.statement.returnToCache = false; } if (this.errorInfo.batchErrors) { this.errorOccurred = false; diff --git a/lib/thin/util.js b/lib/thin/util.js index b74918356..78fe76619 100644 --- a/lib/thin/util.js +++ b/lib/thin/util.js @@ -63,86 +63,41 @@ function getMetadataMany(vars) { } //--------------------------------------------------------------------------- -// getOutBinds(sql) +// getOutBindValues(sql) // -// Get the outBinds for the sql +// Return the values for the out binds at the given position (row). //--------------------------------------------------------------------------- -function getOutBinds(bindVars, pos) { +function getOutBindValues(bindVars, pos) { const bindByPos = (bindVars[0].name === undefined); - let outBinds; - if (bindByPos) { - outBinds = []; - } else { - outBinds = {}; - } + const outBindValues = (bindByPos) ? [] : {}; for (let i = 0; i < bindVars.length; i++) { - if (bindVars[i].dir === constants.BIND_IN) - continue; if (bindByPos) { - outBinds.push(bindVars[i].values[pos]); + outBindValues.push(bindVars[i].values[pos]); } else { - outBinds[bindVars[i].name] = bindVars[i].values[pos]; + outBindValues[bindVars[i].name] = bindVars[i].values[pos]; } } - return outBinds; -} - -//--------------------------------------------------------------------------- -// getExecuteManyOutBinds(sql) -// -// Get the outBinds for the sql when doing an executeMany call -//--------------------------------------------------------------------------- -function getExecuteManyOutBinds(bindVars, numIters) { - const numOutBinds = getNumOutBinds(bindVars); - if (numOutBinds === 0) { - return; - } - const outBinds = new Array(numIters).fill(null); - for (let i = 0; i < numIters; i++) { - outBinds[i] = getOutBinds(bindVars, i); - } - return outBinds; + return outBindValues; } //--------------------------------------------------------------------------- -// getNumOutBinds(sql) +// getOutBinds() // -// Get the number of outBinds for the sql -//--------------------------------------------------------------------------- -function getNumOutBinds(bindVars) { - let numOutBinds = 0; - for (let i = 0; i < bindVars.length; i++) { - if (bindVars[i].dir !== constants.BIND_IN) { - numOutBinds++; +// Return the out binds for the given statement. +//--------------------------------------------------------------------------- +function getOutBinds(statement, numIters, executeManyFlag) { + const bindVars = statement.bindInfoList.map(i => i.bindVar); + const outBinds = bindVars.filter(v => v.dir !== constants.BIND_IN); + if (outBinds.length > 0) { + if (executeManyFlag) { + const outBindValues = new Array(numIters); + for (let i = 0; i < numIters; i++) { + outBindValues[i] = getOutBindValues(outBinds, i); + } + return outBindValues; } + return getOutBindValues(outBinds, 0); } - return numOutBinds; -} - -//--------------------------------------------------------------------------- -// getExecuteOutBinds(sql) -// -// Get the outBinds for the sql when doing an execute call -//--------------------------------------------------------------------------- -function getExecuteOutBinds(bindVars) { - const numOutBinds = getNumOutBinds(bindVars); - if (numOutBinds === 0) { - return; - } - return getOutBinds(bindVars, 0); -} - -//--------------------------------------------------------------------------- -// getOutBinds(sql) -// -// Get the bind variables from the bindInfoList of the statement object -//--------------------------------------------------------------------------- -function getBindVars(statement) { - const bindVars = []; - for (const bindInfo of statement.bindInfoList) { - bindVars.push(bindInfo.bindVar); - } - return bindVars; } //--------------------------------------------------------------------------- @@ -286,10 +241,7 @@ function checkCredentials(params) { module.exports = { getMetadataMany, CLIENT_INFO, - getExecuteOutBinds, - getExecuteManyOutBinds, getOutBinds, - getBindVars, checkProxyUserValidity, checkCredentials }; diff --git a/src/njsSodaOperation.c b/src/njsSodaOperation.c index 2e115891a..e96e7617e 100644 --- a/src/njsSodaOperation.c +++ b/src/njsSodaOperation.c @@ -391,6 +391,7 @@ static bool njsSodaOperation_processOptions(njsBaton *baton, napi_env env, napi_value options) { dpiVersionInfo versionInfo; + bool lock; // allocate memory for ODPI-C operations structure baton->sodaOperOptions = calloc(1, sizeof(dpiSodaOperOptions)); @@ -433,6 +434,8 @@ static bool njsSodaOperation_processOptions(njsBaton *baton, napi_env env, if (!njsUtils_getNamedPropertyString(env, options, "hint", &baton->hint, &baton->hintLength)) return false; + if (!njsUtils_getNamedPropertyBool(env, options, "lock", &lock)) + return false; // populate SODDA operations options structure baton->sodaOperOptions->filter = baton->filter; @@ -446,6 +449,7 @@ static bool njsSodaOperation_processOptions(njsBaton *baton, napi_env env, baton->sodaOperOptions->keyLengths = baton->keysLengths; baton->sodaOperOptions->hint = baton->hint; baton->sodaOperOptions->hintLength = (uint32_t) baton->hintLength; + baton->sodaOperOptions->lock = (int) lock; return true; }