From 550cc139f5ec5512e0d4c3640843d739cef6dd3c Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Thu, 1 Dec 2016 22:46:40 +1100 Subject: [PATCH] More LOB tweaks: fix a mem leak with NULL LOBs; consistently return NULL for string/buffer of EMPTY_LOB; Auto-close (where possible) the IN-bit of a LOB bind used for BIND_INOUT --- src/njs/src/njsConnection.cpp | 111 +++++++++++++++++++++++++--------- src/njs/src/njsIntLob.h | 6 +- test/devnull.js | 50 +++++++++++++++ test/list.txt | 2 +- test/lobBind1.js | 28 ++++----- 5 files changed, 150 insertions(+), 47 deletions(-) create mode 100644 test/devnull.js diff --git a/src/njs/src/njsConnection.cpp b/src/njs/src/njsConnection.cpp index 5936df90..dc978d5c 100644 --- a/src/njs/src/njsConnection.cpp +++ b/src/njs/src/njsConnection.cpp @@ -1960,6 +1960,26 @@ void Connection::Async_Execute (uv_work_t *req) executeBaton->dpistmt->release (); } + // Auto close the IN-LOB used for INOUT bind + for ( unsigned int index = 0 ;index < executeBaton->binds.size (); + index++ ) + { + Bind *bind = executeBaton->binds[index]; + + if ( ( bind->type == DpiClob || bind->type == DpiBlob ) && bind->isInOut ) + { + if ( executeBaton->extBinds[index] && + executeBaton->extBinds[index]->type == NJS_EXTBIND_LOB && + !executeBaton->extBinds[index]->fields.extLob.isStringBuffer2LOB ) + { + ILob *iLob = ( ILob * ) + ( executeBaton->extBinds[index]->fields.extLob.value ); + // cleanupNJS() will be called later in Async_AfterExecute() + iLob->cleanupDPI (); + } + } + } + // In case of error, free the allocated resources if ( !(executeBaton->error).empty() ) { @@ -2103,7 +2123,11 @@ void Connection::LOB2StringOrBuffer ( eBaton* executeBaton, unsigned int index, lobLocator, byteAmount, charAmount, offset, bind->value, bufLen ); - // There is more data in the LOB than the maxSize, set the error + /* + * byteAmount returns the number of bytes read into the buffer irrespective + * of charAmount or byteAmount passed to Lob::read() + * If there is more data in the LOB than the maxSize, set the error + */ if ( byteAmount > ( unsigned long long ) bind->maxSize ) { executeBaton->error = NJSMessages::getErrorMsg( @@ -2111,6 +2135,12 @@ void Connection::LOB2StringOrBuffer ( eBaton* executeBaton, unsigned int index, goto exitLOB2StringOrBuffer; } + // Treat empty LOB case as NULL to be consistent with varchar/buffer columns + if ( !byteAmount ) + { + *bind->ind = -1; + } + *bind->len = ( DPI_BUFLEN_TYPE ) byteAmount; exitLOB2StringOrBuffer: @@ -3390,40 +3420,58 @@ void Connection::Descr2protoILob( eBaton *executeBaton, unsigned int numCols, { Bind *bind = executeBaton->binds[obndpos]; - if ((!bind->isOut && !bind->isInOut) || - ( bind->ind && *(sb2 *)bind->ind == -1)) - continue; // we only need to process the non-null OUT binds - - if (bind->type == DpiClob || bind->type == DpiBlob) + if ( ( bind->isOut || bind->isInOut ) && + ( bind->type == DpiClob || bind->type == DpiBlob ) ) { - Descriptor *lobLocator = NULL; - - if ( executeBaton->stmtIsReturning ) + // Free the allocated descriptors in case of NULL OUTs or DML returning + if ( *bind->ind == -1 ) { - // TODO: Update this loop when support for array binds is added. - // For UPDATE, loop through all the rows returned for each bind - // For INSERT, eventually we will consider the iters. - // For now only 1 row will be inserted. - for ( unsigned int rowidx = 0; rowidx < bind->rowsReturned; rowidx++ ) + if ( executeBaton->stmtIsReturning ) { - lobLocator = - ( ( Descriptor ** ) bind->value )[ rowidx ]; - ProtoILob *protoILob = new ProtoILob ( executeBaton, lobLocator, - bind->type ); - - ( ( Descriptor ** )( bind->value ) )[rowidx] = - reinterpret_cast( protoILob ); + for ( unsigned int rowidx = 0; rowidx < bind->rowsReturned; + rowidx++ ) + { + Env::freeDescriptor ( ( ( Descriptor ** ) bind->value )[ rowidx ], + LobDescriptorType); + } + } + else + { + Env::freeDescriptor ( *( ( Descriptor ** ) bind->value ), + LobDescriptorType); } } - else // case for PLSQL INOUT or OUT + else { - lobLocator = *( ( Descriptor ** ) bind->value ); + Descriptor *lobLocator = NULL; - ProtoILob *protoILob = new ProtoILob ( executeBaton, - lobLocator, - bind->type ); - *((Descriptor **)bind->value) = - reinterpret_cast( protoILob ); + if ( executeBaton->stmtIsReturning ) + { + // TODO: Update this loop when support for array binds is added. + // For UPDATE, loop through all the rows returned for each bind + // For INSERT, eventually we will consider the iters. + // For now only 1 row will be inserted. + for ( unsigned int rowidx = 0; rowidx < bind->rowsReturned; rowidx++ ) + { + lobLocator = + ( ( Descriptor ** ) bind->value )[ rowidx ]; + ProtoILob *protoILob = new ProtoILob ( executeBaton, lobLocator, + bind->type ); + + ( ( Descriptor ** )( bind->value ) )[rowidx] = + reinterpret_cast( protoILob ); + } + } + else + { + lobLocator = *( ( Descriptor ** ) bind->value ); + + ProtoILob *protoILob = new ProtoILob ( executeBaton, + lobLocator, + bind->type ); + *((Descriptor **)bind->value) = + reinterpret_cast( protoILob ); + } } } } @@ -3465,6 +3513,13 @@ void Connection::Async_AfterExecute(uv_work_t *req) ( executeBaton->extBinds[index]->fields.extLob.value ); iLob->postBind (); + // Auto-close the IN-LOB used for INOUT bind + if ( bind->isInOut ) + { + // cleanupDPI() called in Async_Execute since it is a DPI/OCI call + iLob->cleanupNJS (); + } + executeBaton->extBinds[index]->fields.extLob.value = NULL; } } diff --git a/src/njs/src/njsIntLob.h b/src/njs/src/njsIntLob.h index e1e8cf63..b587369a 100644 --- a/src/njs/src/njsIntLob.h +++ b/src/njs/src/njsIntLob.h @@ -185,14 +185,16 @@ class ILob : public Nan::ObjectWrap static void Init(Handle target); + void cleanupDPI (); + + void cleanupNJS (); + private: ILob(); ~ILob(); - void cleanupDPI (); - void cleanupNJS (); inline NJSErrorType getErrNumber ( bool processBind ); static NAN_METHOD(New); diff --git a/test/devnull.js b/test/devnull.js new file mode 100644 index 00000000..3d83a5b1 --- /dev/null +++ b/test/devnull.js @@ -0,0 +1,50 @@ +/* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. */ + +/****************************************************************************** + * + * You may not use the identified files except in compliance with the Apache + * License, Version 2.0 (the "License.") + * + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0. + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * + * See the License for the specific language governing permissions and + * limitations under the License. + * + * The node-oracledb test suite uses 'mocha', 'should' and 'async'. + * See LICENSE.md for relevant licenses. + * + * NAME + * devnull.js + * + * DESCRIPTION + * /dev/null for Node streams + * + *****************************************************************************/ +'use strict'; + +var stream = require('stream'); +var util = require('util'); + +module.exports = DevNull; + +// step 2 - to call the Writable constructor in our own constructor +function DevNull(opts) { + if ( !(this instanceof DevNull) ) return new DevNull(opts); + + opts = opts || {}; + stream.Writable.call(this, opts); + +}; + +// step 1 - to extend the Writable Class +util.inherits(DevNull, stream.Writable); + +// step 3 -define a '_write()' method in the prototype of our stream object +DevNull.prototype._write = function(chunk, encoding, cb) { + setImmediate(cb); +}; diff --git a/test/list.txt b/test/list.txt index 0412c958..36a76651 100644 --- a/test/list.txt +++ b/test/list.txt @@ -1055,7 +1055,7 @@ Overview of node-oracledb functional tests 71.1.4 BIND_IN, PL/SQL, a txt file 71.1.5 BIND_OUT, PL/SQL, a string 71.1.6 BIND_OUT, PL/SQL, a txt file - 71.1.7 BIND_INOUT, PL/SQL, A String + 71.1.7 BIND_INOUT, PL/SQL, A String. IN LOB gets auto closed. 71.1.8 BIND_INOUT, PL/SQL, a txt file 71.1.9 Negative: BIND_INOUT, PL/SQL, String as bind IN Value 71.2 persistent BLOB diff --git a/test/lobBind1.js b/test/lobBind1.js index 4a1b6fd8..b8a96988 100644 --- a/test/lobBind1.js +++ b/test/lobBind1.js @@ -39,6 +39,7 @@ var should = require('should'); var async = require('async'); var dbConfig = require('./dbconfig.js'); var assist = require('./dataTypeAssist.js'); +var devnull = require('./devnull'); describe('71. lobBind1.js', function() { @@ -579,7 +580,7 @@ describe('71. lobBind1.js', function() { }); // 71.1.6 - it('71.1.7 BIND_INOUT, PL/SQL, A String. IN LOB value does not change.', function(done) { + it('71.1.7 BIND_INOUT, PL/SQL, A String. IN LOB gets auto closed.', function(done) { var seq = 7; var inStr = "I love the sunshine today!", @@ -621,13 +622,13 @@ describe('71. lobBind1.js', function() { should.not.exist(err); (result.rows.length).should.not.eql(0); - var lob = result.rows[0][0]; + var lobin = result.rows[0][0]; connection.execute( sql2, { id: seq, - io: { val: lob, type: oracledb.CLOB, dir: oracledb.BIND_INOUT } + io: { val: lobin, type: oracledb.CLOB, dir: oracledb.BIND_INOUT } }, { autoCommit: true }, function(err, result2) { @@ -636,7 +637,7 @@ describe('71. lobBind1.js', function() { var lobout = result2.outBinds.io; async.parallel([ - function(callback) { + function verifyOUT(callback) { lobout.setEncoding("utf8"); var clobData = ""; @@ -654,23 +655,18 @@ describe('71. lobBind1.js', function() { return callback(); }); }, - function(callback) { - lob.setEncoding("utf8"); - var clobData = ""; + function verifyIN(callback) { - lob.on("data", function(chunk) { - clobData += chunk; - }); + lobin.pipe(devnull()); - lob.on("error", function(err) { - should.not.exist(err, "lob.on 'error' event."); - }); - - lob.on("end", function() { - should.strictEqual(clobData, inStr); + lobin.on('error', function(err) { + should.exist(err); + (err.message).should.startWith('NJS-022:'); + // Error: NJS-022: invalid Lob return callback(); }); + } ], function(err) { should.not.exist(err);