WIP: 836 - pool.close() should iterate open connections and close them

[danilohgds]

Added NJS-064 to util.js file
Added logic for an extra parameter in pool.js to forcibly close connection pool with DPI_MODE_POOL_CLOSE_FORCE after a timer has passed.

Signed-off-by: Danilo Henrique Garcia da Silva danilo_hgds@hotmail.com

[danilohgds]

@cjbj

• You may not need to actually use the DPI constant names as strings - maybe something simpler is better?

I thought of changing closeMode to forceClose and use it as a boolean, pass it over to the .cpp layer and then before the work is queued, the actual DPI enum value is resolved based on the variable.

• I think that connection.close() should be checking if _isPoolClosing is true and if the number of open connections is 0. If those conditions are true, then the pool can be closed immediately

I think it would overcomplicate things if we were to check isPoolClosing on the connection.close calls as well. Checking for connection == 0 in the pool.close call seems to be enough imo.

• We should discuss whether during the shutdown timer period you should stop all methods being called from the JS layer except things like release(), commit(), rollback(). We may not want new work to start but we definitely want users to be able to rollback or commit work and free up resources.

I went with the option of blocking only new getConnection calls from being executed and returned an NJS-064 to the user. Are there any other scenarios where we should block work?

• Should _isPoolClosing be a public pool attribute so that apps can check it if they want?

I don't see a reason not to. I will change it.

[dmcghan]

@danilohgds Maybe rename isPoolClosing to just closing? "pool" seems superflous as it's an attribute of the pool: if (myPool.closing) {...}.

We had some attributes that used to start with "is", such as isAutoCommit and isExternalAuth which are now autoCommit and externalAuth. Maybe it's best to just leave the "is" out.

[danilohgds]

@dmcghan I changed them over to only closing. Initially I went with the poolClosing name as many variables are referenced that way, example poolMin, poolMax, and they get referenced the same way in cpp, so I followed that pattern.

[danilohgds]

@cjbj @anthony-tuininga the changes on the c++ layer are also included. I tested some cases which I came up with and the changes seem good.
Any feedback please let me know.

[danilohgds]

@anthony-tuininga
Thanks for the feedback, I changed it over, please take a look :)

[danilohgds]

@anthony-tuininga I copied it over from your example, I ended up not seeing the Object part was missing.

[anthony-tuininga]

Right. Formatting issue. I forgot to tell it that it was code. Sorry about that!

[cjbj]

Shouldn't this allow drainTime of 0, meaning force the connection pool to close?

[danilohgds]

Yeah, I meant to have that as 0, good catch.

[cjbj]

@danilohgds Looks good. Shall we take it over and test it and do doc etc?

[danilohgds]

@cjbj please do.

[cjbj]

@danilohgds Excellent.

Anthony is already doing ODPI-C changes to make sure the forced pool close doesn't cause problems.

[danilohgds]

@cjbj Awesome! shall I move to the next feature then ✌️ ? I was looking around the enhancement labels, do u have any in mind?

[cjbj]

@danilohgds Scanning my long "to do" list, most changes are C++ related. What level of work are you up for?

If you want small 'paper cut' things:

• Add connectString as readable property of a Connection to help debugging & tracing
• NJS-040 Message Text: Include queuetimeout name & value
• NJS-011 Message Text: include name/position information
• NJS-009 Message Text: add number of parameters seen and expected
• "NJS-051: given data as bind value is too large" should contain dpi_max_buflen
• Make username an alias for user in the connect properties

Or what about adding some async/await examples? I already have a variant of webapp.js that is awaiting @dmcghan's review, but other examples would be good.

[danilohgds]

@cjbj I want to tackle the scary stuff, but my C++ isn't yet up to the task. I will take a look at those small ones first then.

I have some stuff I wrote with async await, maybe I can run those by @dmcghan and then see which are useful and what not. I'm always up for adding examples!

[cjbj]

@danilohgds a bigger example showing GraphQL or a REST service might be good. Specially if it has comments about how to use it in Angular!

[cjbj]

What about returning this in the callback?

-  nodbUtil.assert(self.closing === false, 'NJS-064', self.poolAlias);
-
+  if (self.closing === true) {
+    getConnectionCb(new Error(nodbUtil.getErrorMessage('NJS-064', self.poolAlias)));
+    return;
+  }

[cjbj]

The question is then, should there be a second message printed when the pool has finally closed? The self.closing attribute would need to have 3 states.

[danilohgds]

can we have a closing and hasClosed property? due to the nature of the closing being a boolean I would like to have a new property in this case.

[cjbj]

@danilohgds I know @anthony-tuininga is keen to add a check on connectionsOut at connection release which may change the flags a bit; I'll let him add his proposal.

[anthony-tuininga]

My suggestion was to store "_closeCb" (callback function) on the pool instead of "closing" (boolean). That would allow, as Chris mentioned, the ability to check connectionsOut at connection release. If the connectionsOut after release is zero, the close callback can be invoked immediately (but first that attribute would be reset to undefined so that when the timeout expires it won't call it a second time). This would permit well-behaved applications with reasonably short connection lifetimes to shut down quickly without waiting for the drain time to expire.

My other suggestion was to change the API to use an options object instead of a simple integer. The options object could contain one or both of these two keys:

• force: forces the pool to be closed even if connections are out (default false)
• drainTime: specifies the maximum amount of time the pool will wait for connections to be released back to the pool (default 0 seconds)

This provides the current behaviour (immediate close with a failure if any connections are out) and also provides a means to control both options. Currently if any value is specified for drainTime then it is assumed that forcing the close at the end of the drainTime value is desirable, and if no value is specified, then forcing is not desired. Its a bit awkward, but workable. Thoughts?

[danilohgds]

@anthony-tuininga I think I understand your suggestion now. So essentially, the driver would be able to intervene and invoke the call back when the connectionsOut reaches 0 and save time. Although, I think that the close callback can be called only after the self._close is called.. because that is when the pool shuts down.

As for the second suggestion, this would essentially mean that I can set up a drain time delay for both force and default option correct? I'm trying to see situations in which setting a drain time without the force would be used, since it might eventually fail if there still connections, maybe a routine that catches such error and schedules the closing again after X time? Might be beneficial I guess.

[cjbj]

I'm fine with the first suggestion (it completes #836 (comment)) but I'm not worried if it gets deferred.

I don't see a strong reason to have a drainTime for a non-force close. Having what is effectively 'named' parameters does add flexibility for future OCI session pool enhancements, but I'm not sure it's worth it. I'd be happy to go with the implemented 'less typing' API.

[danilohgds]

I changed some of the code around for proper error messaging like you suggested, also cleaned up some variables and comments.

I will see how to make the ._close function stored and callable later during the connection releases.

[cjbj]

@danilohgds excellent. Ping us if you want us to take over.

[cjbj]

Some of the asserts are printing the wrong parameter numbers. How about moving them all to the top like this (based on how execute() does its parameter checks):

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

  nodbUtil.assert(arguments.length === 1 || arguments.length === 2, 'NJS-009');
  switch (arguments.length) {
  case 1:
    nodbUtil.assert(typeof a1 === 'function', 'NJS-006', 1);
    closeCb = a1;
    break;
  case 2:
    nodbUtil.assert(typeof a1 === 'number', 'NJS-006', 1);
    nodbUtil.assert(typeof a2 === 'function', 'NJS-006', 2);
    drainTime = a1 * 1000;  // milliseconds
    closeCb = a2;
    // SetTimeout does not accept numbers greater than a 32-bit signed integer.
    const upperLimitTimeout = 2147483647;
    nodbUtil.assert(drainTime >= 0 && drainTime <= upperLimitTimeout, 'NJS-005', 1);
    break;
  }

  if (arguments.length === 1) {
    // The default close mode will only succeed if all connections have been checked in.
    self.closeWrapper(false, closeCb);
  } else {
    // Force close the pool.
    self.closing = true;

    // If a drainTime has been provided, it will use that value, given that connections count is not 0.
    // If connectionsOut is === 0, closePool is called right away, as there is no need to wait at all.
    drainTime = self._connectionsOut === 0 ? 0 : drainTime;

    if (drainTime === 0) {
      self.closeWrapper(true, closeCb);
    } else {
      setTimeout(() => {
        self.closeWrapper(true, closeCb);
      }, drainTime);
    }
  }
}

I have't extensively tested it yet.

[cjbj]

@danilohgds if you're bored, one other general node-oracledb enhancement request would be to update the typescript files.

[cjbj]

What about moving the setting of the closing property to closeWrapper()? It's a straight assignment of the value of mode.

[cjbj]

Or maybe not. It should probably be set before making an async call

[cjbj]

I guess this new property should be added to _logStats() too:

  console.log('...closing:', self.closing);

[danilohgds]

I agree, that information might be helpful in some cases.
As for the closing, I thought of changing it over to an enum:

*not closing
*close pending
*close issued

Not only it does give a better sense of what is going on under the hood, but also allows execute callbacks to cut in and call close first should there be no connectionsOut in use.

What do you think?

[cjbj]

Seems fair.

[cjbj]

@danilohgds A proposed update landed on https://github.com/oracle/node-oracledb/tree/danilohgds-PR-836 Take a look.

[cjbj]

@danilohgds I pushed this in 4999982. Take a look and see what cleanup you want. A few tweaks from myself, @anthony-tuininga and @octokyle crept in :) Can you review the pool.close() doc?

I changed the NJS message text so the pool alias isn't printed: not all pools have aliases. If you think the messages and logic should be updated to handle both cases (with & without aliases), send in the PR!

[danilohgds]

@cjbj Looks fine to me. I think showing pool alias is redudant since you have to get the pool by the alias in case you have more than one.
I'm really happy the pool made it to 3.0

[cjbj]

@danilohgds We really appreciate you putting time into this and making it happen. If you're in the office hours session tomorrow we can catch up.

[danilohgds]

@cjbj I will be there ☺.

[cjbj]

Since this is now in master, I'll close the PR. Thanks @danilohgds.