Replace generic-pool with tarn

[dhensby]

Rebase of #624

generic-pool does seem to be pretty broken from what I can tell (see #805)

Tarn JS seems reasonable but it doesn't appear to be actively maintained (last release was 1 year ago)

[dhensby]

Given the response to coopernurse/node-pool#256 (that it is intentional that resources continue to be created / released after draining the pool) we probably should look at moving away from generic-pool because this is just not compatible with a pool of DB connections. If the factoryCreate method is going to fail and it is not possible to stop it, we can only expect to be stuck in a blocking loop and there's nothing we can do about it because that's it working as intended.

[dhensby]

@patriksimek it seems that the travis builds aren't running or being triggered properly - any chance you can look at it?

---

Ah, don't worry - I have access - I can't see anything amiss at the moment

[dhensby]

OK - I think travis is not building draft PRs for some reason

[gshively11]

hi @dhensby, we just got bit by the generic-pool loop issue. i was thinking about trying to do a patch
similar to this, until an official fix lands in upstream. what are your thoughts on this approach?

[dhensby]

@gshively11 that looks interesting, though I've tried a patch of my own here: #806

I'm not entirely following how the patch you linked to solves the problem because if we lose the DB connection we'll error immediately and that will win the promise "race"...

As explained above, unfortunately generic-pool just appears to be broken for our needs anyway because beyond this issue (immediate retry of resource creation on failure) there's no way to stop this loop once it starts because closing the pool doesn't prevent the pool from trying to create new resources that started before close was called.

This means if we detect a connection error and close the pool the library will continue to create a resource until the acquire timeout is reached (if it's set) or infinitely until the DB comes back.

[dhensby]

I'm not entirely following how the patch you linked to solves the problem because if we lose the DB connection we'll error immediately and that will win the promise "race"...

OK - so the idea is that the pool error would have been recorded before the next tick picks up the timer and evaluates the promise.. ok, JS is funky.

[gshively11]

Yea, something like that. I'm going to play with it today to see how it behaves. Even if the error doesn't map 1:1 with an acquire call, if we can propagate the error up to tedious and prevent an infinite loop, that'll be good enough for our use case.

[dhensby]

@gshively11 would you mind testing out my fix in #806 ?

It's not going to fix an infinite loop problem if there's no acquire timeout set on the pool but it stops the pool trying to create hundreds of connections a second.

the only other option I can think of is a fairly aggressive destruction of the pool on connection errors, which seems a bit over the top.

[gshively11]

Yea will do, although we definitely want to fix both problems.

[dhensby]

Yes, but the infinite loop problem can only be fixed by:

1. Replacing the pool with one more suited to our needs (this PR)
2. Set an acquire timeout (recommended already)
3. Aggressively closing the pool on connection errors (I've not even explored how to do this)

You can already attempt to close the pool on connection errors:

pool.connect().then(connection => {
    connection.on('error', (e) => {
        if (connection.connected || connection.connecting) {
            connection.close()
        }
    });
});

This can be used in connection with an acquire timeout to stop pools getting stuck.

[gshively11]

1. We want to stay on v4 for a while, I think the pool replacement is in v5/6, yea? I'm hoping we can get a patch to fix all generic-pool's issues.
2. Yep, should definitely do that
3. Making sure the pools are closed is definitely needed.

We want to stop generic-pool from getting stuck in an infinite dispatch loop. I think we can do that similar to how sequelize did it. Does #806 handle that piece as well, or is it only making sure that pools get closed and connection attempts aren't spammed?

[dhensby]

We want to stay on v4 for a while, I think the pool replacement is in v5/6, yea? I'm hoping we can get a patch to fix all generic-pool's issues.

That's right, but I'd recommend a v5 upgrade as there isn't any significant breaking changes, it adds pausable stream support to requests and (soon) to fix this pool issue.

We want to stop generic-pool from getting stuck in an infinite dispatch loop.

At the moment no, because the only way to do that is to swallow any errors and release a dud connection object. That would be quite a significant change to how this lib works that would certainly require a major version bump. It would also mean one bad connection has the ability to trash the entire pool...

[gshively11]

At the moment no, because the only way to do that is to swallow any errors and release a dud connection object. That would be quite a significant change to how this lib works that would certainly require a major version bump. It would also mean one bad connection has the ability to trash the entire pool...

Ok, I think we'll do a quick temporary fork just for our stuff, because a dud connection object is way more desirable than the current state. We have a wrapper around node-mssql that does failover/retry etc to handle that situation anyway. Once we're stable again I can test out v5 and the lib replacement fix.

[gshively11]

Ok, I thought I was going to be able to work a hack in node-mssql to make generic-pool work, but I don't think I can without monkey patching. So I need to focus on either patching/forking generic-pool, or looking at your tarn solution. I'm looking for the internal pool used by node-mssql to function as follows, could you let me know if this is how it'll be in your PR plz @dhensby

• create instance of ConnectionPool, call connect
• if initial probe fails, connect rejects, nothing else happens
• if initial probe succeeds, internal Pool is created
• a request is initiated, acquire is called
  • now we get to the pool logic
  • an existing resource is retrieved, validate is called and flags it as invalid (or no resources are available and a new one is created, if so skip the next step)
  • invalid resource is destroyed and create is called
  • create fails to connect to the db
  • the acquire call is rejected, no further attempts to create are made for this request
  • the request gets rejected by the rejected acquire
• another request is initiated, acquire is called
  • repeat

this behavior is critical due to how we handle password rotations, failovers, etc. for our databases. we cannot have the pool indefinitely try to recreate. it can try once per request, but then it needs to bubble that failure up so we can take action on it (kill the ConnectionPool, create a new one with new DB config)

[gshively11]

It seems like it does, since you're using propagateCreateError. I'm going to play around with this branch locally to see if it fixes our issues.

[gshively11]

@dhensby moving the conversation into #812

[gshively11]

on line 262 of tedious.js, would we need to add a check for hasPromiseResolved there as well? we don't want to call reject if we've already resolved the promise. we would also need to set hasPromiseResolved to true inside the end event's reject call.

[dhensby]

on line 262 of tedious.js, would we need to add a check for hasPromiseResolved there as well? we don't want to call reject if we've already resolved the promise. we would also need to set hasPromiseResolved to true inside the end event's reject call.

I've made it so that the reject/resolve calls can only be done once without need for a variable to be passed around

[dhensby]

Ideally for v6 we'd also upgrade the tedious version (again)

---

See #818

[willmorgan]

Looks good. A changelog entry should be added, along with a short migration guide.

Could we augment the pool property with size, available, pending and borrowed as gettable variables that emit deprecation warnings and return the tarn equivalents? I can think of at least one project that uses those props in production 😉

---

Edit: just to clarify, I'm talking about using Object.defineProperty with a getter, on the pool property, and having that getter emit a deprecation warning:

1. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty
2. https://nodejs.org/api/process.html#process_process_emitwarning_warning_type_code_ctor

[dhensby]

along with a short migration guide

There's actually very little migration required because the config API is so similar. The main changes are accessing the properties and if I add in those properties it'll basically it.

I'm not sure where would be appropriate to put the upgrade guide, either :/

[willmorgan]

I'm not sure where would be appropriate to put the upgrade guide, either :/

I would stick it in the changelog and/or the GH pages branch where there's a 3-4x migration guide already.

[willmorgan]

@dhensby Thanks for actioning/answering feedback, this just needs rebasing now (annoyingly!) but otherwise it looks ready to go from my point of view.

[dhensby]

@willmorgan rebased :)