Node-oracledb: `pool.close()` should iterate open connections and close them

[jsumners]

This is very similar to #760.

Basically, the documentation for pool.close() says all connections retrieved from the pool must be closed before invoking pool.close(). It seems to me that getConnection() should be able to track the connections it issues so that when the pool shutsdown it can close those connections.

[anthony-tuininga]

The OCI library has the ability to force the shutdown of the pool and forces all connections closed. This is not plumbed through to the node-oracledb driver but could be. Where this enhancement sits on the long list of enhancements is something that @cjbj will have to comment on.

[cjbj]

Upcoming changes to ODPI-C are a step towards allowing this in node-oracledb.

[mohawk2]

@cjbj The above looks like it's been released now as 2.2.0, so this might be closable now?

[cjbj]

@mohawk2 not yet.

[danilohgds]

@cjbj I want to work on this one, any pointers on where to begin?

[cjbj]

@danilohgds we had a 2-minute internal discussion about this last week and I wrote up this as a draft proposal:

We could implement connection pool draining in NJS with two calls to close the pool:

• The first dpiPool_close uses DPI_MODE_POOL_CLOSE_DEFAULT
• If this returns an error because connections are still 'checked out' then:
  
  
  
  • set a flag so future getConnection calls know to return an error without attempting to call the C++ layer
  
  
  • set a timer to initiate a second dpiPool_close call using DPI_MODE_POOL_CLOSE_FORCE after a user specified time
  
  
  • immediately expire all getConnection requests in the pool queue
  
  

• The API would become

Callback:

close([drainTime,] function(Error error){}); 

Promise:

promise = close([drainTime]); 

Where drainTime is seconds. The default would be -1, meaning no drain time i.e. the current behavior where all connections must be explicitly closed before the pool will shut down. A drainTime 0 means immediate close with DPI_MODE_POOL_CLOSE_FORCE

How does it sound? Note the description at the top is for when drainTime > 0. And there are some unexplored issues about how, or if, connections that are forcibly closed can be handled gracefully.

Happy coding!

[jsumners]

I think that is a fair compromise.

[cjbj]

The 'returns an error' is probably bogus, but the general use of a timer and two calls is probably feasible.

[cjbj]

@danilohgds here are some more thoughts after another discussion:

Prototype:

Callback: pool.close([drainTime,] function(Error error){});
Promise: promise = pool.close([drainTime]);

Description:

• Sets JS timer to call dpiPool_close with DPI_MODE_POOL_CLOSE_FORCE after drainTime seconds
• Any new getConnection() calls fail immediately in JS layer with new error:
  "NJS-064: Connection cannot be created because the connection pool is closing"
• Any pending connection requests in the pool queue immediately return NJS-064.
• conn.close() calls behave as current: releasing sessions back to OCI Session Pool
• If self._connectionsOut == 0 before the timer fires, then call dpiPool_close immediately
• After dpiPool_close with DPI_MODE_POOL_CLOSE_FORCE then
  
  
  
  • using any connection, e.g. for execute() or LOBs should fail (maybe in JS layer or ODPI or both?)
  
  
  • apps that want to continue running nicely should call conn.close() for any open connections.
  
  
  • conn.close() calls should not call OCISessionRelease. This may be best done in ODPI-C (which we would change)
  
  

If drainTime is not passed:

• Option 1
  
  
  
  • Backward compatible current behavior e.g. use
    
    DPI_MODE_POOL_CLOSE_DEFAULT and never set a timer etc. Apps
    
    that want the pool closed can check for connectionsInUse == 0
    
    before calling pool.close()
  
  

• Option 2
  
  
  
  • Use a 60 (?) second drainTime. If apps don't want the forced
    
    close they can wait for connectionsInUse == 0 before calling
    
    pool.close()
  
  

[danilohgds]

This is what I got so far, I wanted to confirm if I'm going on the right track:

function close(a1, a2) {
  var self = this;

  nodbUtil.assert(arguments.length >= 1 && arguments.length <= 2, 'NJS-009');
  nodbUtil.assert(typeof closeCb === 'function', 'NJS-006', 1);

  var closeCallback = arguments[arguments.length - 1];
  var closeMode = arguments.length === 1 ? 'DPI_MODE_POOL_CLOSE_DEFAULT' : 'DPI_MODE_POOL_CLOSE_FORCE';

  if(arguments.length === 1){
    self._close(closeMode,function(err) {
      if (!err) {
        self._isValid = false;

        self.emit('_after_close', self);
      }  
      closeCallback(err);
    });
  }else{
    var drainTime = self._connectionsOut === 0 ? 1 : arguments[arguments.length - 2];

    setTimeout(() => {
      self._close(closeMode,function(err) {
        if (!err) {
          self._isValid = false;

          self.emit('_after_close', self);
        }

        closeCallback(err);
      });
    }, drainTime);
  }
}

If drainTime was not supplied, go with backwards compatibility and normal close mode.
If drainTime is supplied, evaluate the connection count, and overwrite it to 0 if there are no connections.
Call the timeout.

I will work on njsPool.cpp to accept that new argument, then work out the rest of the things we discussed. Also, any ideas on how to signal that the pool is closing is appreciated, I'm thinking of setting a flag when close is called.

• Sets JS timer to call dpiPool_close with DPI_MODE_POOL_CLOSE_FORCE after drainTime seconds : done
• Any new getConnection() calls fail immediately in JS layer with new error
  "NJS-064: Connection cannot be created because the connection pool is closing" : TBD
• Any pending connection requests in the pool queue immediately return NJS-064. : TBD
• conn.close() calls behave as current: releasing sessions back to OCI Session Pool : unchanged
• If self._connectionsOut == 0 before the timer fires, then call dpiPool_close immediately : done
• After dpiPool_close with DPI_MODE_POOL_CLOSE_FORCE then using any connection, e.g. for execute() or LOBs should fail (maybe in JS layer or ODPI or both?) I think that changing it in JS Layer is enough.

[dmcghan]

@danilohgds In the JS layer, we always check the number and type of parameters (basic sanity checks) passed to public APIs. The assertion routines get a bit more complex once the number of parameters becomes variable. See getConnection for examples.

• I see you updated the check for the parameter count, that looks good. But the second assert call is checking closeCb which no longer exists (declared later as closeCallback).
• You'll need to add an assert for the type of drainTime (probably a number) which will execute if that parameter is passed in.
• Most other validations are done in the C layer (though I'd love to see those moved to the JS layer). You might add an additional check on the actual value of drainTime - is there a min or max?

I see you're using an arrow function expression. I'm fine with that now that we only have to support Node.js 6+, but we should probably start using let and const over var too (maybe that could all be cleaned up in a separate PR). Frankly, we should probably adopt a JS style and enforce it with ESLint. I'm not a fan of Standard (though I could be convinced), but we could experiment with other style guides from Google, Air B&B, and others. Again, that's another PR.

The logic in each branch of the if block is the same except that one is in a setTimeout call. Consider moving that logic to a separate named function and invoking it where needed.

Also, any ideas on how to signal that the pool is closing is appreciated, I'm thinking of setting a flag when close is called.

Is this related to the last bullet which starts with: "After dpiPool_close with..."? If so, a flag sounds resonable, but the question is where should it go? I'm guessing the C layer because I can't figure out, for example, how you'd go from a LOB > conn > pool to check it. Maybe I'm confused. :)

Just curious... You set the closeMode to DPI_MODE_POOL_CLOSE_FORCE if 2 parameters are passed in. Is it valid for me to invoke and pass a drainTime of 0 to essentially force the pool closed without a wait?

[danilohgds]

I see you updated the check for the parameter count, that looks good. But the second assert call is checking closeCb which no longer exists (declared later as closeCallback).

An early draft from the code I was writing, I got that issue fixed. Also, some other optimizations are already done from my side.

Most other validations are done in the C layer (though I'd love to see those moved to the JS layer). You might add an additional check on the actual value of drainTime - is there a min or max?

From a programming perspective, the minimum is 1 millisecond and the maximum is 2147483647 since setTimeout uses a signed 32bit integer to represent the milliseconds, I assume we could do something on that regard and block any non-conforming value.

About the arrow functions, I was about to ask that question as a followup. Given that we have dropped support for versions older than node6, no reason that we should not enjoy the goodies of ES6 syntax. I am a huge fan of the Airbnb style guide, so if you want I can include ts-lint within my PR no prob, and you guys can see what fits best in the development style you have.

About the flag, what about if we export a flag to process.env to signal that pool is closing. Everywhere from the code should have access to that without any problem.

Just curious... You set the closeMode to DPI_MODE_POOL_CLOSE_FORCE if 2 parameters are passed in. Is it valid for me to invoke and pass a drainTime of 0 to essentially force the pool closed without a wait?

What I get from the flag is the following: the default flag will close the pool, only if there are no active sessions, else it will throw an error, while the FORCE flag does cause all connections to be closed first.
I assume that passing "drainTime" of 0 means you want to immediately (1 ms actually ) cease and existing connection and then the pool, I believe that the timer gives some control over when the poll gets forcibly shut which is nice.

The last thing... are PR's to the C layer well received? Or are those classes maintained by Oracle only like the ODPI stuff.

[dmcghan]

I am a huge fan of the Airbnb style guide, so if you want I can include ts-lint within my PR no prob, and you guys can see what fits best in the development style you have.

It's probably best to keep that as a separate PR. It will need buy-in from the team, time to review style guides, etc.

About the flag, what about if we export a flag to process.env to signal that pool is closing.

What if you have more than one pool?

I assume that passing "drainTime" of 0 means you want to immediately (1 ms actually ) cease and existing connection and then the pool, I believe that the timer gives some control over when the poll gets forcibly shut which is nice.

I think it would be nice to support a "force" close without a wait. Given the current API, a drainTime of 0 feels the most "natural". Is there a reason to force a minimum of 1?

The last thing... are PR's to the C layer well received?

Good question for @cjbj and @anthony-tuininga.

[danilohgds]

What if you have more than one pool?

Welp! More thought needed on this one,

I think it would be nice to support a "force" close without a wait. Given the current API, a drainTime of 0 feels the most "natural". Is there a reason to force a minimum of 1?

SetTimeout doesn't work with less than 1 millisecond, though I can write the logic to call the draintime right away without any waits.

[danilohgds]

@cjbj might wanna close this as it has been placed in 3.00, right?

[cjbj]

@danilohgds I have (mostly) been closing enhancement issues only when the new release is installable.

[cjbj]

Node-oracledb 3.0 has just been released with the PR from @danilohgds to allow pool draining and session closing. Take a look!