Allow repeat calls to ConnectionPool.connect()

[dhensby]

Depends on #940

closes #934

This PR implements the feature proposal outlined in #934 - it allows developers to make repeat calls to the ConnectionPool.connect() function (and have them resolve as appropriate) even if the pool is in a connected or connecting state.

This has been achieved by:

1. Immediately resolving callbacks if the pool is in a connected state
2. Stacking connect callbacks so that they can be resolved in parallel once the connection promise has resolved

This does not touch on the further enhancements in #934, namely there is no auto-connect feature added, as this feels a lot more risky and the behaviour under certain edge-cases is harder to define.

todo:

• Docs

[dhensby]

Last thing to consider is what to do if a ConnectionPool is closing and a connect call is made. Should connect throw, or should it just queue up a new connection?

If users start running code like pool.connect().then(() => pool.query()).then(() => pool.close()) in parallel, we will be able to handle it, but it will be an absolutely terrible way to use the library because it will be creating and destroying pools all the time including running the probe connection before connect() resolves.

For now, I've allowed this usage, but maybe we should throw an error?

[dhensby]

Perhaps I can add some more thorough tests to how the base pool handles connecting / closing of the pool

[dhensby]

I'm starting to stumble upon some of the issues with allowing repeat calls to connect especially if it means we start to allow / encourage free usage of pool.connect().then(() => pool.request().query()).then(() => pool.close()) - because this means there's no longer a guarantee that the pool will be connected once the connected call resolves. That is because the close call can execute after a connection and query has run but not after all queries have run (and we will never know this).

Here's a failing case:

const pool = new sql.ConnectionPool(config)
      Promise.all([
        pool.connect().then(() => pool.request().query('SELECT 1')).then(() => pool.close()),
        pool.connect()
          .then(() => pool.request().query('SELECT 2'))
          // here the pool has been closed
          .then(() => pool.request().query('SELECT 3'))
          .then(() => pool.close()),
      ]).then(() => {
        done()
      }).catch(done)

My expectation is that we don't encourage this use of connect and close, but it is something people might be tempted to do and could lead to connect resolving a pool that isn't connected under some circumstances. Perhaps this is just an edge case to be aware of, because I still think it's best that connect resolves even if the pool is already connected.

An alternative would be that if close is called whilst connect is executing, we could actually reject all the connection promises so they can't resolve and instead error... In that instance the above then calls after the connect would never resolve

[willmorgan]

My expectation is that we don't encourage this use of connect and close, but it is something people might be tempted to do and could lead to connect resolving a pool that isn't connected under some circumstances. Perhaps this is just an edge case to be aware of, because I still think it's best that connect resolves even if the pool is already connected.

I think a better way for these kinds of cases is to start by questioning why they're using a pool to begin with, if they're doing what would appear to be one-off operations which imply only a single connection is going to be used. In that case only a single connection should be required and pooling is overkill.

[brokenthorn]

Absolutely! Why would you open an entire pool of connections just to run a few tasks with any one of those tasks or all of them wanting to close the entire pool after they are finished? That's just bad code design. You would close the pool in the `Promise.all([...]).then( () => pool.close() )` last `then`, __after__ all tasks have finished using the pool.

…

On Mon, Nov 18, 2019 at 3:27 PM Will Morgan ***@***.***> wrote: My expectation is that we don't encourage this use of connect and close, but it is something people might be tempted to do and could lead to connect resolving a pool that isn't connected under some circumstances. Perhaps this is just an edge case to be aware of, because I still think it's best that connect resolves even if the pool is already connected. I think a better way for these kinds of cases is to start by questioning why they're using a pool to begin with, if they're doing what would appear to be one-off operations which imply only a single connection is going to be used. In that case only a single connection should be required and pooling is overkill. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#941?email_source=notifications&email_token=AAQFKXA7ZG7UOWPIF6Y33BDQUKJ2VA5CNFSM4JM5H6YKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEKNSOQ#issuecomment-555014458>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQFKXB7W2VYVXFNQONNPALQUKJ2VANCNFSM4JM5H6YA> .

[dhensby]

I mean, you can argue that repeatedly calling connect is bad code design too!

I might spend a bit more time looking into this, though...