Sending a command to a nonexistent database fails quietly and the promise is never rejected

[koskimas]

The following standalone app demonstrates the issue. This happens with knex 0.8.3. Didn't happen with 0.7.3.

var knex = require('knex')({
  client: 'pg',
  connection: {
    host: '127.0.0.1',
    database: 'does_not_exist'
  },
  pool: {
    min: 0,
    max: 10
  }
});

knex.raw('SELECT 42').then(function () {
  console.log('this is never printed');
}).catch(function () {
  console.log('neither is this');
});

// Prevent the program from exiting.
setTimeout(function () {}, 10000);

[tgriesser]

I'll take a look

[myndzi]

10 seconds is not long enough to wait. The database connect timeout can be extremely long (I just got done troubleshooting a network connectivity issue that was sourced here). If you wait at least 60 seconds (or more?) does it still stall?

[koskimas]

It should reject the promise as soon as postgres responds with an error (which happens immediately). Waiting 60 seconds is not an option in my case.

[tgriesser]

I see that there's an issue here - looking exactly at what's the cause, I think it's the use of .disposer, since the connection is never acquired, the query is never run.

However, I think it may actually might sense to change the behavior here - presumably you're looking for a way to deal with the connection should it not exist. Sending a test query is sort of a hack to accomplish that.

@koskimas what are you ultimately trying to accomplish with the SELECT 42 query - are you looking to somehow eventually handle the invalid connection state and reconnect?

[tgriesser]

Actually so @myndzi - in Pool2, the pool is automatically destroyed if the error is called:

// throw an error if we haven't successfully allocated a resource yet
if (this.live === false) {
  this._destroyPool();
  err.message = 'Error allocating resources: ' + err.message;
  this.emit('error', err);
}

Should this happen? What if it was just a fluke that one of the allocations failed but subsequent ones succeed?

[koskimas]

The example I gave is just a something I wrote to demonstrate the bug. I've built a library around knex for helping with automated testing. It can be used to easily create, drop, truncate and access multiple databases. I use the database drivers (pg, sqlite, etc) directly to some of the stuff that can't be done with knex. Anyways, my library's interface guarantees that the promises are rejected in error cases (that can easily happen while writing tests).

I don't have any actual use case where I need this functionality, but I think its weird if operations just silently fail, no matter what the error is.

[tgriesser]

but I think its weird if operations just silently fail, no matter what the error is.

Totally - currently the 'error' event coming from the pool is intercepted, logging an error to the console rather than crashing, but I could just proxy the error event through to the client and have it bubble up to the knex instance where it would actually crash the instance if unhandled.

Thoughts?

[myndzi]

The .live property is set to true when the first successful resource allocation is made; until then it is false. This is to help distinguish the case of "I've configured the pool but haven't asked for any resources, and now I asked for a resource but it turns out there's a problem and I can't/will never get any resources" (e.g. database misconfiguration) from other cases. The pool is only destroyed if resource allocation fails for the first time, basically.

P.S. - if pg responds with an error, yeah, you should receive an error. I was referring in this case to errors that are connection timeouts. Sorry for the confusion :)

[koskimas]

Crashing the instance means crashing the node instance? This would be an improvement and most likely the correct way to handle this in most cases. I'm still not getting why can't the promise just be rejected? How about the case where the database server is temporarily down? We don't want to crash the app, but return a 500 error code or something else that requires the error to be caught. Or is this handled differently?

[myndzi]

I've fixed the basic problem here (not rejecting requests when destroying the pool) and pushed a new patch version to NPM. Can you update 'pool2' in your knex module (reinstalling knex should be fine) and see if the problem still occurs?

[myndzi]

koskimas:

Since we're using a resource pool here, an error acquiring a resource is not directly associated with a specific query that wants a resource to perform its function. The current behavior of pool2 is to retry indefinitely; essenitally, requests for a resource go into a queue, and as long as there are requests pending, the pool tries to serve them. It doesn't reject requests on a singular error acquiring a resource, because there's nothing tying the two together yet -- they are only tied together after the pooler gives the resource to the request.

It occurs to me that not providing any sort of timeout/fail functionality for requests is probably an oversight and I'm inclined to add an option to cover that. I started a ticket for further discussion here, and would welcome feedback:

https://github.com/myndzi/pool2/issues/7

To answer your individual questions, the current behavior is like so:

If you provide invalid database data, your request and all future requests will be rejected; the first request will reject whenever the failure happens, and the pool will be destroyed; after that, subsequent requests will also reject.

If the database server is temporarily down, assuming you've had at least one successful request, further requests will stall indefinitely until the database server responds again.

[koskimas]

@myndzi The update fixed my problem. Thank you!

[myndzi]

Great! :)

[diversario]

If the database server is temporarily down, assuming you've had at least one successful request, further requests will stall indefinitely until the database server responds again.

So, in the event of db failover – does pool discard existing (and now bad) connections and acquires new ones?

[myndzi]

This depends a bit on a number of things; presumably, existing connections will emit errors or otherwise notify event handlers bound when the "acquire resource" function is called (this function is provided by knex on configuring the pool). These handlers should then remove the resource from the pool. New resources will be acquired when it is possible (open resources < max) to serve pending requests.

In short, yes, connections will be closed as they emit errors or otherwise notify the allocating function, and new connections will be acquired as long as the configuration continues to work.

[diversario]

"acquire resource" function is called (this function is provided by knex on configuring the pool).

Could you explain how to use this function? I don't think knexjs.org has anything about it.

[myndzi]

It's internals. The pool is a generic pool. You give it a function to acquire a resource, and a function to destroy a resource, and then just ask it for stuff and it uses those functions to provide. That's how database connections get allocated to serve queries; you don't use it directly, but knex uses it on your behalf.

[myndzi]

See: https://github.com/tgriesser/knex/blob/master/lib/dialects/postgres/index.js#L67-68

Knex binds an 'error' and 'end' event handler; these are emitted by the database driver when there's an error or when a connection closes. Knex's error handlers then tell the pool to "get rid of" these resources down here:

https://github.com/tgriesser/knex/blob/master/lib/dialects/postgres/index.js#L160-L166

The pool calls this function to "get the job done":

https://github.com/tgriesser/knex/blob/master/lib/dialects/postgres/index.js#L82-L85

And does some internal bookkeeping to drop that resource from its list of available resources. This leaves "free spots" if there weren't already, which are filled if there aren't any idle resources and a new request for a resource comes in.