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

Author: Christopher Jones

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,14 +2123,24 @@ 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(
                                           errInsufficientBufferForBinds);
     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<Descriptor *>( 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;
+
+        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 );
 
-        ProtoILob *protoILob          = new ProtoILob ( executeBaton,
-                                                        lobLocator,
-                                                        bind->type );
-        *((Descriptor **)bind->value) =
-                                  reinterpret_cast<Descriptor *>( protoILob );
+            ( ( Descriptor ** )( bind->value ) )[rowidx] =
+              reinterpret_cast<Descriptor *>( protoILob );
+          }
+        }
+        else
+        {
+          lobLocator                    = *( ( Descriptor ** ) bind->value );
+
+          ProtoILob *protoILob          = new ProtoILob ( executeBaton,
+                                                          lobLocator,
+                                                          bind->type );
+          *((Descriptor **)bind->value) =
+                                    reinterpret_cast<Descriptor *>( 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;
       }
     }

src/njs/src/njsIntLob.h

@@ -185,14 +185,16 @@ class ILob : public Nan::ObjectWrap
 
   static void Init(Handle<Object> target);
 
+  void cleanupDPI ();
+
+  void cleanupNJS ();
+
 
  private:
   ILob();
 
   ~ILob();
 
-  void cleanupDPI ();
-  void cleanupNJS ();
   inline NJSErrorType getErrNumber ( bool processBind );
 
   static NAN_METHOD(New);

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);
+};

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

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 = "";
-
-                      lob.on("data", function(chunk) {
-                        clobData += chunk;
-                      });
+                    function verifyIN(callback) {
 
-                      lob.on("error", function(err) {
-                        should.not.exist(err, "lob.on 'error' event.");
-                      });
+                      lobin.pipe(devnull());
 
-                      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);