Skip to content

Commit a43d59a

Browse files
cjbjcjbj
committed
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.
1 parent d86254d commit a43d59a

File tree

14 files changed

+1370
-598
lines changed

14 files changed

+1370
-598
lines changed

‎lib/lob.js

Lines changed: 29 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,16 @@ function Lob(iLob, opt, oracledb) {
3232

3333
this.iLob = iLob;
3434

35-
if (!iLob.tempLob) {
36-
this.once('finish', this._closeSync);
37-
}
38-
3935
Object.defineProperties(
4036
this,
4137
{
4238
_oracledb: { // _oracledb property used by promisify () in util.js
4339
value: oracledb
4440
},
41+
_autoCloseLob: { // Tells whether to close at the end of stream or not
42+
value: iLob.autoCloseLob,
43+
writable: false
44+
},
4545
chunkSize: {
4646
value: iLob.chunkSize,
4747
writable: false
@@ -64,20 +64,16 @@ function Lob(iLob, opt, oracledb) {
6464
return iLob.type;
6565
}
6666
},
67-
tempLob: {
68-
get: function() {
69-
return iLob.tempLob;
70-
},
71-
set: function() {
72-
throw new Error(nodbUtil.getErrorMessage('NJS-014', 'tempLob'));
73-
}
74-
},
7567
close: {
7668
value: closePromisified,
7769
enumerable: true,
7870
writable: true
7971
}
8072
});
73+
74+
if (this._autoCloseLob) {
75+
this.once('finish', this._closeSync);
76+
}
8177
}
8278

8379
Lob.prototype._read = function() {
@@ -86,24 +82,29 @@ Lob.prototype._read = function() {
8682
self.iLob.read(
8783
function(err, str) {
8884
if (err) {
89-
// Ignore if any error occurs during close
90-
// Emits 'close' event after closing LOB
91-
self._closeSync();
85+
if (self._autoCloseLob) {
86+
// Ignore if any error occurs during close
87+
// Emits 'close' event after closing LOB
88+
self._closeSync();
89+
}
9290
self.emit('error', err);
9391
return;
9492
}
9593

9694
self.push(str);
9795

98-
if (!str) {
99-
process.nextTick(function() {
100-
err = self._closeSync(); // Emits 'close' event after closing LOB
96+
if (self._autoCloseLob) {
97+
if (!str) {
98+
process.nextTick(function() {
99+
err = self._closeSync(); // Emits 'close' event after closing LOB
101100

102-
if (err) {
103-
self.emit('error', err);
104-
}
101+
if (err) {
102+
self.emit('error', err);
103+
return;
104+
}
105105

106-
});
106+
});
107+
}
107108
}
108109
}
109110
);
@@ -148,6 +149,12 @@ Lob.prototype.close = function(closeCb) {
148149
nodbUtil.assert(arguments.length === 1, 'NJS-009');
149150
nodbUtil.assert(typeof closeCb === 'function', 'NJS-006', 1);
150151

152+
// Return if LOB already closed to support multiple close() calls should be
153+
// no-op
154+
if (!self.iLob.valid) {
155+
return closeCb(null);
156+
}
157+
151158
self.iLob.close(function(err) {
152159
if (!err) {
153160
self.emit('close');

‎src/dpi/include/dpiLob.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
#ifndef DPILOB_ORACLE
3131
# define DPILOB_ORACLE
3232

33+
#ifndef OCI_ORACLE
34+
# include <oci.h>
35+
#endif
3336

3437
#ifndef DPICOMMON_ORACLE
3538
# include <dpiCommon.h>
@@ -90,8 +93,11 @@ class Lob
9093
static void createTempLob ( DpiHandle *svch, DpiHandle *errh,
9194
Descriptor *lobLocator, unsigned char lobType );
9295

93-
static void freeTempLob ( DpiHandle *envh, DpiHandle *svch,
94-
DpiHandle *errh, Descriptor *lobLocator );
96+
static void freeTempLob ( DpiHandle *svch, DpiHandle *errh,
97+
Descriptor *lobLocator );
98+
99+
static boolean isTempLob ( DpiHandle *envh, DpiHandle *errh,
100+
Descriptor *lobLocator );
95101
};
96102

97103

‎src/dpi/src/dpiLob.cpp

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@
2727
*
2828
*****************************************************************************/
2929

30-
#ifndef DPIUTILS_ORACLE
31-
# include <dpiUtils.h>
30+
#ifndef DPILOB_ORACLE
31+
# include <dpiLob.h>
3232
#endif
3333

3434

35-
#ifndef DPILOB_ORACLE
36-
# include <dpiLob.h>
35+
#ifndef DPIUTILS_ORACLE
36+
# include <dpiUtils.h>
3737
#endif
3838

3939

@@ -267,7 +267,6 @@ void Lob::createTempLob ( DpiHandle *svch, DpiHandle *errh,
267267
Frees a temporary LOB
268268
269269
PARAMETERS
270-
envh - OCI ENV handle
271270
svch - OCI service handle
272271
errh - OCI error handle
273272
lobLocator - Lob locator
@@ -277,21 +276,40 @@ void Lob::createTempLob ( DpiHandle *svch, DpiHandle *errh,
277276
nothing
278277
279278
*/
280-
void Lob::freeTempLob ( DpiHandle *envh, DpiHandle *svch, DpiHandle *errh,
279+
void Lob::freeTempLob ( DpiHandle *svch, DpiHandle *errh,
281280
Descriptor *lobLocator )
281+
{
282+
ociCall ( OCILobFreeTemporary ( ( OCISvcCtx * ) svch, ( OCIError * ) errh,
283+
( OCILobLocator * ) lobLocator ),
284+
( OCIError * ) errh );
285+
}
286+
287+
288+
/*******************************************************************************
289+
290+
DESCRIPTION
291+
Check whether given LOB is temporary or not
292+
293+
PARAMETERS
294+
envh - OCI ENV handle
295+
errh - OCI error handle
296+
lobLocator - Lob locator
297+
298+
299+
RETURNS
300+
boolean - true if LOB is temporary
301+
false otherwise
302+
*/
303+
boolean Lob::isTempLob ( DpiHandle *envh, DpiHandle *errh,
304+
Descriptor *lobLocator )
282305
{
283306
boolean isTemporary = FALSE;
284307

285308
ociCall ( OCILobIsTemporary ( ( OCIEnv * ) envh, ( OCIError * ) errh,
286309
( OCILobLocator * ) lobLocator, &isTemporary ),
287310
( OCIError * ) errh );
288311

289-
if ( isTemporary )
290-
{
291-
ociCall ( OCILobFreeTemporary ( ( OCISvcCtx * ) svch, ( OCIError * ) errh,
292-
( OCILobLocator * ) lobLocator ),
293-
( OCIError * ) errh );
294-
}
312+
return isTemporary;
295313
}
296314

297315

‎src/njs/src/njsConnection.cpp

Lines changed: 43 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1304,10 +1304,7 @@ void Connection::GetInBindParamsScalar(Local<Value> v8val, Bind* bind,
13041304
executeBaton->njsconn->InitExtBind ( extBind, NJS_EXTBIND_LOB );
13051305

13061306
// Store the reference to IN LOB object
1307-
extBind->fields.extLob.value =
1308-
( void * ) malloc ( sizeof ( Descriptor * ) );
1309-
*(Descriptor **)extBind->fields.extLob.value =
1310-
reinterpret_cast<Descriptor *>(iLob);
1307+
extBind->fields.extLob.value = (void *) iLob;
13111308

13121309
// create a persistent reference to avoid un-expected GC
13131310
Nan::Persistent<Object> *jsBindObj;
@@ -1987,7 +1984,6 @@ void Connection::Async_Execute (uv_work_t *req)
19871984
if ( !executeBaton->binds[index]->isOut )
19881985
{
19891986
Lob::freeTempLob (
1990-
executeBaton->dpienv->envHandle(),
19911987
executeBaton->dpiconn->getSvch(),
19921988
executeBaton->dpiconn->getErrh (),
19931989
*( Descriptor** ) executeBaton->binds[index]->value );
@@ -2118,7 +2114,13 @@ void Connection::LOB2StringOrBuffer ( eBaton* executeBaton, unsigned int index,
21182114
*bind->len = ( DPI_BUFLEN_TYPE ) byteAmount;
21192115

21202116
exitLOB2StringOrBuffer:
2121-
// Free the allocated LOB descriptor and memory for out bind
2117+
if ( Lob::isTempLob ( executeBaton->dpienv->envHandle(),
2118+
executeBaton->dpiconn->getErrh (), lobLocator ) )
2119+
{
2120+
Lob::freeTempLob ( executeBaton->dpiconn->getSvch(),
2121+
executeBaton->dpiconn->getErrh (),
2122+
lobLocator );
2123+
}
21222124
Env::freeDescriptor ( lobLocator, LobDescriptorType );
21232125
if ( !executeBaton->error.empty() )
21242126
{
@@ -2268,11 +2270,12 @@ void Connection::Descr2StringOrBuffer ( eBaton* executeBaton )
22682270
}
22692271
else
22702272
{
2271-
// Free the temp LOB created for IN bind
2272-
Lob::freeTempLob ( executeBaton->dpienv->envHandle (),
2273-
executeBaton->dpiconn->getSvch (),
2274-
executeBaton->dpiconn->getErrh (),
2275-
*( Descriptor** ) bind->value );
2273+
if ( *bind->ind != -1 )
2274+
{
2275+
Lob::freeTempLob ( executeBaton->dpiconn->getSvch (),
2276+
executeBaton->dpiconn->getErrh (),
2277+
*( Descriptor** ) bind->value );
2278+
}
22762279

22772280
// Free the allocated LOB descriptor
22782281
Env::freeDescriptor( *(Descriptor **) bind->value,
@@ -2392,8 +2395,8 @@ void Connection::PrepareLOBsForBind ( eBaton* executeBaton, unsigned int index )
23922395
executeBaton->extBinds[index]->type == NJS_EXTBIND_LOB &&
23932396
!executeBaton->extBinds[index]->fields.extLob.isStringBuffer2LOB )
23942397
{
2395-
ILob *iLob = ( ILob * ) ( * ( static_cast<ILob**>
2396-
( executeBaton->extBinds[index]->fields.extLob.value ) ) );
2398+
ILob *iLob = ( ILob * )
2399+
( executeBaton->extBinds[index]->fields.extLob.value );
23972400

23982401
if ( bind->isInOut )
23992402
{
@@ -3458,9 +3461,11 @@ void Connection::Async_AfterExecute(uv_work_t *req)
34583461
executeBaton->extBinds[index]->type == NJS_EXTBIND_LOB &&
34593462
!executeBaton->extBinds[index]->fields.extLob.isStringBuffer2LOB )
34603463
{
3461-
ILob *iLob = ( ILob * ) ( * ( static_cast<ILob**>
3462-
( executeBaton->extBinds[index]->fields.extLob.value ) ) );
3464+
ILob *iLob = ( ILob * )
3465+
( executeBaton->extBinds[index]->fields.extLob.value );
34633466
iLob->postBind ();
3467+
3468+
executeBaton->extBinds[index]->fields.extLob.value = NULL;
34643469
}
34653470
}
34663471
}
@@ -4212,10 +4217,6 @@ ConnectionBusyStatus Connection::getConnectionBusyStatus ( Connection *conn )
42124217
connStatus = NJS_CONN_BUSY_RS;
42134218
else if ( conn->dbCount_ != 1 ) // 1 for Release operaion itself
42144219
connStatus = NJS_CONN_BUSY_DB;
4215-
else if ( conn->tempLobCount_ != 0 )
4216-
{
4217-
connStatus = NJS_CONN_BUSY_TEMPLOB;
4218-
}
42194220

42204221
return connStatus;
42214222
}
@@ -4275,9 +4276,6 @@ NAN_METHOD(Connection::Release)
42754276
case NJS_CONN_BUSY_DB:
42764277
releaseBaton->error = NJSMessages::getErrorMsg( errBusyConnDB );
42774278
break;
4278-
case NJS_CONN_BUSY_TEMPLOB:
4279-
releaseBaton->error = NJSMessages::getErrorMsg( errBusyConnTEMPLOB );
4280-
break;
42814279
}
42824280

42834281
exitRelease:
@@ -4347,7 +4345,19 @@ void Connection::Async_AfterRelease(uv_work_t *req)
43474345
argv[0] = v8::Exception::Error(
43484346
Nan::New<v8::String>(releaseBaton->error).ToLocalChecked());
43494347
else
4350-
argv[0] = Nan::Undefined();
4348+
{
4349+
// Populate error if Temporary LOBs are still open
4350+
if ( releaseBaton->njsconn->tempLobCount_ != 0 )
4351+
{
4352+
releaseBaton->error = NJSMessages::getErrorMsg( errBusyConnTEMPLOB );
4353+
argv[0] = v8::Exception::Error(
4354+
Nan::New<v8::String>(releaseBaton->error).ToLocalChecked());
4355+
}
4356+
else
4357+
{
4358+
argv[0] = Nan::Undefined();
4359+
}
4360+
}
43514361

43524362
/*
43534363
* When we release the connection, we have to clear the reference of
@@ -4847,8 +4857,7 @@ void Connection::Async_AfterCreateLob (uv_work_t *req)
48474857
createLobBaton->lobInfo->lobLocator,
48484858
lobType );
48494859

4850-
// The last parameter sets isTempLob_ to true
4851-
Local<Value> value = NewLob ( createLobBaton, protoILob, true );
4860+
Local<Value> value = NewLob ( createLobBaton, protoILob, false );
48524861

48534862
delete protoILob;
48544863

@@ -5344,12 +5353,13 @@ int Connection::cbDynBufferGet ( void *ctx, DPI_SZ_TYPE nRows,
53445353
Create a new LOB object.
53455354
53465355
PARAMETERS
5347-
executeBaton - eBaton struct containing execute context info
5348-
protoILob - ProtoILob object created using LOB locator in the process
5349-
of creating ILob object
5350-
isTempLob - boolean
5351-
true - in case of temporary LOB
5352-
false - otherwise
5356+
executeBaton - eBaton struct containing execute context info
5357+
protoILob - ProtoILob object created using LOB locator in the process
5358+
of creating ILob object
5359+
isAutoCloseLob - bool
5360+
true - the Lob object will be closed at end of streaming
5361+
operation
5362+
false - otherwise
53535363
53545364
RETURNS
53555365
a Lob object
@@ -5372,7 +5382,7 @@ int Connection::cbDynBufferGet ( void *ctx, DPI_SZ_TYPE nRows,
53725382

53735383
v8::Local<v8::Value> Connection::NewLob( eBaton *executeBaton,
53745384
ProtoILob *protoILob,
5375-
bool isTempLob )
5385+
bool isAutoCloseLob )
53765386
{
53775387
Nan::EscapableHandleScope scope;
53785388
Connection *connection = executeBaton->njsconn;
@@ -5390,7 +5400,7 @@ v8::Local<v8::Value> Connection::NewLob( eBaton *executeBaton,
53905400
// handles in the ILob cleanup routine.
53915401

53925402
(Nan::ObjectWrap::Unwrap<ILob>(iLob))->setILob ( executeBaton, protoILob,
5393-
isTempLob );
5403+
isAutoCloseLob );
53945404

53955405
if (!executeBaton->error.empty())
53965406
return Nan::Null();

‎src/njs/src/njsConnection.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -238,8 +238,8 @@ typedef struct eBaton
238238
bool extendedMetaData;
239239
MetaInfo *mInfo;
240240
LobInfo *lobInfo;
241-
std::vector<Nan::Persistent<Object>*> persistentRefs; // Persistent Refs to
242-
// JS Objects
241+
std::vector<Nan::Persistent<Object>*>
242+
persistentRefs; // Persistent Refs to JS Objects
243243

244244
eBaton( unsigned int& count, Local<Function> callback,
245245
Local<Object> jsConnObj ) :
@@ -566,7 +566,7 @@ class Connection: public Nan::ObjectWrap
566566
// NewLob Method on Connection class
567567
static v8::Local<v8::Value> NewLob( eBaton* executeBaton,
568568
ProtoILob *protoILob,
569-
bool tempLob = false );
569+
bool isAutoCloseLob = true );
570570

571571
static inline ValueType GetValueType ( v8::Local<v8::Value> v )
572572
{

0 commit comments

Comments
 (0)