Fixed LOB review comments

Make multiple lob.close() calls no-ops to be more resilient and allow defensive coding.

Allow connections to be closed when temp LOBs are still open: a warning is given since LOBs may leak.

Treat temporary LOBs returned from DB the same as persistent LOBs.  This allowed the planned lob.tempLob property to be removed, simplifying LOB usage.

Author: Christopher Jones

lib/lob.js

@@ -32,16 +32,16 @@ function Lob(iLob, opt, oracledb) {
 
   this.iLob = iLob;
 
-  if (!iLob.tempLob) {
-    this.once('finish', this._closeSync);
-  }
-
   Object.defineProperties(
     this,
     {
       _oracledb: { // _oracledb property used by promisify () in util.js
         value: oracledb
       },
+      _autoCloseLob: { // Tells whether to close at the end of stream or not
+        value: iLob.autoCloseLob,
+        writable: false
+      },
       chunkSize: {
         value: iLob.chunkSize,
         writable: false
@@ -64,20 +64,16 @@ function Lob(iLob, opt, oracledb) {
           return iLob.type;
         }
       },
-      tempLob: {
-        get: function() {
-          return iLob.tempLob;
-        },
-        set: function() {
-          throw new Error(nodbUtil.getErrorMessage('NJS-014', 'tempLob'));
-        }
-      },
       close: {
         value: closePromisified,
         enumerable: true,
         writable: true
       }
     });
+
+  if (this._autoCloseLob) {
+    this.once('finish', this._closeSync);
+  }
 }
 
 Lob.prototype._read = function() {
@@ -86,24 +82,29 @@ Lob.prototype._read = function() {
   self.iLob.read(
     function(err, str) {
       if (err) {
-        // Ignore if any error occurs during close
-        // Emits 'close' event after closing LOB
-        self._closeSync();
+        if (self._autoCloseLob) {
+          // Ignore if any error occurs during close
+          // Emits 'close' event after closing LOB
+          self._closeSync();
+        }
         self.emit('error', err);
         return;
       }
 
       self.push(str);
 
-      if (!str) {
-        process.nextTick(function() {
-          err = self._closeSync(); // Emits 'close' event after closing LOB
+      if (self._autoCloseLob) {
+        if (!str) {
+          process.nextTick(function() {
+            err = self._closeSync(); // Emits 'close' event after closing LOB
 
-          if (err) {
-            self.emit('error', err);
-          }
+            if (err) {
+              self.emit('error', err);
+              return;
+            }
 
-        });
+          });
+        }
       }
     }
   );
@@ -148,6 +149,12 @@ Lob.prototype.close = function(closeCb) {
   nodbUtil.assert(arguments.length === 1, 'NJS-009');
   nodbUtil.assert(typeof closeCb === 'function', 'NJS-006', 1);
 
+  // Return if LOB already closed to support multiple close() calls should be
+  // no-op
+  if (!self.iLob.valid) {
+    return  closeCb(null);
+  }
+
   self.iLob.close(function(err) {
     if (!err) {
       self.emit('close');

src/dpi/include/dpiLob.h

@@ -30,6 +30,9 @@
 #ifndef DPILOB_ORACLE
 # define DPILOB_ORACLE
 
+#ifndef OCI_ORACLE
+# include <oci.h>
+#endif
 
 #ifndef DPICOMMON_ORACLE
 # include <dpiCommon.h>
@@ -90,8 +93,11 @@ class Lob
   static void createTempLob ( DpiHandle *svch, DpiHandle *errh,
                               Descriptor *lobLocator, unsigned char lobType );
 
-  static void freeTempLob   ( DpiHandle *envh, DpiHandle  *svch,
-                              DpiHandle *errh, Descriptor *lobLocator );
+  static void freeTempLob   ( DpiHandle  *svch, DpiHandle *errh,
+                              Descriptor *lobLocator );
+
+  static boolean isTempLob  ( DpiHandle *envh, DpiHandle *errh,
+                              Descriptor *lobLocator );
 };
 
 

src/dpi/src/dpiLob.cpp

@@ -27,13 +27,13 @@
  *
  *****************************************************************************/
 
-#ifndef DPIUTILS_ORACLE
-# include <dpiUtils.h>
+#ifndef DPILOB_ORACLE
+# include <dpiLob.h>
 #endif
 
 
-#ifndef DPILOB_ORACLE
-# include <dpiLob.h>
+#ifndef DPIUTILS_ORACLE
+# include <dpiUtils.h>
 #endif
 
 
@@ -267,7 +267,6 @@ void Lob::createTempLob ( DpiHandle *svch, DpiHandle *errh,
     Frees a temporary LOB
 
   PARAMETERS
-    envh            - OCI ENV handle
     svch            - OCI service handle
     errh            - OCI error handle
     lobLocator      - Lob locator
@@ -277,21 +276,40 @@ void Lob::createTempLob ( DpiHandle *svch, DpiHandle *errh,
     nothing
 
 */
-void Lob::freeTempLob ( DpiHandle *envh, DpiHandle *svch, DpiHandle *errh,
+void Lob::freeTempLob ( DpiHandle *svch, DpiHandle *errh,
                         Descriptor *lobLocator )
+{
+  ociCall ( OCILobFreeTemporary ( ( OCISvcCtx * ) svch, ( OCIError * ) errh,
+                                  ( OCILobLocator * ) lobLocator ),
+            ( OCIError * ) errh );
+}
+
+
+/*******************************************************************************
+
+  DESCRIPTION
+    Check whether given LOB is temporary or not
+
+  PARAMETERS
+    envh            - OCI ENV handle
+    errh            - OCI error handle
+    lobLocator      - Lob locator
+
+
+  RETURNS
+    boolean -  true if LOB is temporary
+               false otherwise
+*/
+boolean Lob::isTempLob ( DpiHandle *envh, DpiHandle *errh,
+                         Descriptor *lobLocator )
 {
   boolean isTemporary = FALSE;
 
   ociCall ( OCILobIsTemporary ( ( OCIEnv * ) envh, ( OCIError * ) errh,
                                 ( OCILobLocator * ) lobLocator, &isTemporary ),
             ( OCIError * ) errh );
 
-  if ( isTemporary )
-  {
-    ociCall ( OCILobFreeTemporary ( ( OCISvcCtx * ) svch, ( OCIError * ) errh,
-                                    ( OCILobLocator * ) lobLocator ),
-              ( OCIError * ) errh );
-  }
+  return isTemporary;
 }
 
 

src/njs/src/njsConnection.cpp

@@ -1304,10 +1304,7 @@ void Connection::GetInBindParamsScalar(Local<Value> v8val, Bind* bind,
             executeBaton->njsconn->InitExtBind ( extBind, NJS_EXTBIND_LOB );
 
             // Store the reference to IN LOB object
-            extBind->fields.extLob.value =
-                                ( void * ) malloc ( sizeof ( Descriptor * ) );
-            *(Descriptor **)extBind->fields.extLob.value =
-                                       reinterpret_cast<Descriptor *>(iLob);
+            extBind->fields.extLob.value = (void *) iLob;
 
             // create a persistent reference to avoid un-expected GC
             Nan::Persistent<Object> *jsBindObj;
@@ -1987,7 +1984,6 @@ void Connection::Async_Execute (uv_work_t *req)
           if ( !executeBaton->binds[index]->isOut )
           {
             Lob::freeTempLob (
-                         executeBaton->dpienv->envHandle(),
                          executeBaton->dpiconn->getSvch(),
                          executeBaton->dpiconn->getErrh (),
                          *( Descriptor** ) executeBaton->binds[index]->value );
@@ -2118,7 +2114,13 @@ void Connection::LOB2StringOrBuffer ( eBaton* executeBaton, unsigned int index,
   *bind->len  = ( DPI_BUFLEN_TYPE ) byteAmount;
 
 exitLOB2StringOrBuffer:
-  // Free the allocated LOB descriptor and memory for out bind
+  if ( Lob::isTempLob ( executeBaton->dpienv->envHandle(),
+                        executeBaton->dpiconn->getErrh (), lobLocator ) )
+  {
+    Lob::freeTempLob ( executeBaton->dpiconn->getSvch(),
+                       executeBaton->dpiconn->getErrh (),
+                       lobLocator );
+  }
   Env::freeDescriptor ( lobLocator, LobDescriptorType );
   if ( !executeBaton->error.empty() )
   {
@@ -2268,11 +2270,12 @@ void Connection::Descr2StringOrBuffer ( eBaton* executeBaton )
         }
         else
         {
-          // Free the temp LOB created  for IN bind
-          Lob::freeTempLob ( executeBaton->dpienv->envHandle (),
-                             executeBaton->dpiconn->getSvch (),
-                             executeBaton->dpiconn->getErrh (),
-                             *( Descriptor** ) bind->value );
+          if ( *bind->ind != -1 )
+          {
+            Lob::freeTempLob ( executeBaton->dpiconn->getSvch (),
+                               executeBaton->dpiconn->getErrh (),
+                               *( Descriptor** ) bind->value );
+          }
 
           // Free the allocated LOB descriptor
           Env::freeDescriptor( *(Descriptor **) bind->value,
@@ -2392,8 +2395,8 @@ void Connection::PrepareLOBsForBind ( eBaton* executeBaton, unsigned int index )
        executeBaton->extBinds[index]->type == NJS_EXTBIND_LOB &&
        !executeBaton->extBinds[index]->fields.extLob.isStringBuffer2LOB )
   {
-    ILob *iLob = ( ILob * ) ( * ( static_cast<ILob**>
-                 ( executeBaton->extBinds[index]->fields.extLob.value ) ) );
+    ILob *iLob = ( ILob * )
+                 ( executeBaton->extBinds[index]->fields.extLob.value );
 
     if ( bind->isInOut )
     {
@@ -3458,9 +3461,11 @@ void Connection::Async_AfterExecute(uv_work_t *req)
            executeBaton->extBinds[index]->type == NJS_EXTBIND_LOB &&
            !executeBaton->extBinds[index]->fields.extLob.isStringBuffer2LOB )
       {
-        ILob *iLob = ( ILob * ) ( * ( static_cast<ILob**>
-                       ( executeBaton->extBinds[index]->fields.extLob.value ) ) );
+        ILob *iLob = ( ILob * )
+                     ( executeBaton->extBinds[index]->fields.extLob.value );
         iLob->postBind ();
+
+        executeBaton->extBinds[index]->fields.extLob.value = NULL;
       }
     }
   }
@@ -4212,10 +4217,6 @@ ConnectionBusyStatus Connection::getConnectionBusyStatus ( Connection *conn )
     connStatus = NJS_CONN_BUSY_RS;
   else if ( conn->dbCount_ != 1 ) // 1 for Release operaion itself
     connStatus = NJS_CONN_BUSY_DB;
-  else if ( conn->tempLobCount_ != 0 )
-  {
-    connStatus = NJS_CONN_BUSY_TEMPLOB;
-  }
 
   return connStatus;
 }
@@ -4275,9 +4276,6 @@ NAN_METHOD(Connection::Release)
     case NJS_CONN_BUSY_DB:
       releaseBaton->error = NJSMessages::getErrorMsg( errBusyConnDB );
       break;
-    case NJS_CONN_BUSY_TEMPLOB:
-      releaseBaton->error = NJSMessages::getErrorMsg( errBusyConnTEMPLOB );
-      break;
   }
 
 exitRelease:
@@ -4347,7 +4345,19 @@ void Connection::Async_AfterRelease(uv_work_t *req)
     argv[0] = v8::Exception::Error(
                 Nan::New<v8::String>(releaseBaton->error).ToLocalChecked());
   else
-    argv[0] = Nan::Undefined();
+  {
+    // Populate error if Temporary LOBs are still open
+    if ( releaseBaton->njsconn->tempLobCount_ != 0 )
+    {
+      releaseBaton->error = NJSMessages::getErrorMsg( errBusyConnTEMPLOB );
+      argv[0] = v8::Exception::Error(
+                Nan::New<v8::String>(releaseBaton->error).ToLocalChecked());
+    }
+    else
+    {
+      argv[0] = Nan::Undefined();
+    }
+  }
 
   /*
    * When we release the connection, we have to clear the reference of
@@ -4847,8 +4857,7 @@ void Connection::Async_AfterCreateLob (uv_work_t *req)
                                            createLobBaton->lobInfo->lobLocator,
                                            lobType );
 
-    // The last parameter sets isTempLob_ to true
-    Local<Value> value = NewLob ( createLobBaton, protoILob, true );
+    Local<Value> value = NewLob ( createLobBaton, protoILob, false );
 
     delete protoILob;
 
@@ -5344,12 +5353,13 @@ int Connection::cbDynBufferGet ( void *ctx, DPI_SZ_TYPE nRows,
     Create a new LOB object.
 
   PARAMETERS
-    executeBaton - eBaton struct containing execute context info
-    protoILob    - ProtoILob object created using LOB locator in the process
-                   of creating ILob object
-    isTempLob    - boolean
-                   true  - in case of temporary LOB
-                   false - otherwise
+    executeBaton   - eBaton struct containing execute context info
+    protoILob      - ProtoILob object created using LOB locator in the process
+                     of creating ILob object
+    isAutoCloseLob - bool
+                     true   - the Lob object will be closed at end of streaming
+                              operation
+                     false  - otherwise
 
   RETURNS
     a Lob object
@@ -5372,7 +5382,7 @@ int Connection::cbDynBufferGet ( void *ctx, DPI_SZ_TYPE nRows,
 
 v8::Local<v8::Value> Connection::NewLob( eBaton    *executeBaton,
                                          ProtoILob *protoILob,
-                                         bool      isTempLob )
+                                         bool      isAutoCloseLob )
 {
   Nan::EscapableHandleScope scope;
   Connection     *connection = executeBaton->njsconn;
@@ -5390,7 +5400,7 @@ v8::Local<v8::Value> Connection::NewLob( eBaton    *executeBaton,
   // handles in the ILob cleanup routine.
 
   (Nan::ObjectWrap::Unwrap<ILob>(iLob))->setILob ( executeBaton, protoILob,
-                                                   isTempLob );
+                                                   isAutoCloseLob );
 
   if (!executeBaton->error.empty())
     return Nan::Null();

src/njs/src/njsConnection.h

@@ -238,8 +238,8 @@ typedef struct eBaton
   bool                      extendedMetaData;
   MetaInfo                  *mInfo;
   LobInfo                   *lobInfo;
-  std::vector<Nan::Persistent<Object>*> persistentRefs; // Persistent Refs to
-                                                        // JS Objects
+  std::vector<Nan::Persistent<Object>*>
+                            persistentRefs; // Persistent Refs to JS Objects
 
   eBaton( unsigned int& count, Local<Function> callback,
            Local<Object> jsConnObj ) :
@@ -566,7 +566,7 @@ class Connection: public Nan::ObjectWrap
   // NewLob Method on Connection class
   static v8::Local<v8::Value> NewLob( eBaton*   executeBaton,
                                       ProtoILob *protoILob,
-                                      bool      tempLob = false );
+                                      bool      isAutoCloseLob = true );
 
   static inline ValueType GetValueType ( v8::Local<v8::Value> v )
   {