Make client._queryable public

[tmtron]

Could you make client._queryable public?

i.e. I know that we can already access this variable, but I guess the _ indicates that we shouldn't, and that this is an implementation detail, that can go away anytime.

The use-case is that users of the lib (e.g. pg-promise), can use this info to decide if the client can be reused (e.g. released back to the pool) or if it is broken and should be killed.

Refrence to issue #680 of pg-promise

[charmander]

Catch the 'error' event instead. Everything that sets _queryable = false emits one, and a client can’t be reused after it emits 'error'. (This is what pg-pool does.)

[tmtron]

So, pg-pool should already remove non-queryable connections automatically from the pool, right?

pg-pool:

  _release (client, idleListener, err) {
  ...
  if (err || this.ending) {
      this._remove(client)
  ...

AFAIK, pg-promise is using pg-pool, so I wonder why the broken connection is then reused.
@vitaly-t can you help here?

[vitaly-t]

Well, I agree that the pool should remove clients that errorred automatically.

[tmtron]

Well, I agree that the pool should remove clients that errorred automatically.

@vitaly-t can you please clarify: From the code that I've pasted above and the comment from @charmander, it seems that pg-pool is already doing the right thing.
So do you think that there is something wrong in pg-pool, or in pg-promise? Or maybe it's just a wrong use or misconfiguration in our own code?

[vitaly-t]

This looks like a bug in the Pool. It must never give us a Client that has _queryable set to false, IMO.

And so I had (within pg-promise) to add kill condition based on that flag when releasing the connection back to the pool, to avoid getting it again on the next request.

@brianc, @charmander What do you think about this, guys?

[charmander]

@vitaly-t I agree that the pool should check for errored clients to avoid misuse, but there’s either an error event that’s being incorrectly suppressed here or another bug where an error event isn’t being emitted when _queryable is set to false (very unlikely). You should fix it by listening for an error, not reading the private field _queryable.

[vitaly-t]

You should fix it by listening for an error, not reading the private field _queryable.

Listening for an error, and ordering to kill the client are two separate things, they are not one block of code. And because of this, the client has to store the state somewhere first, and only then use it to signal the pool to kill the connection. This is very awkward, and therefore should be done on the lower level, by the same code which contains the dead connections, which in our case is the pool. The pool should never allow a successful request for a connection that's marked as non-queryable.

[charmander]

@vitaly-t Like I said, I agree on what the pool should do, but in the meantime you should store the state yourself and not rely on a private field (the underscore means it’s private).

[tmtron]

I think when pull#2081 works as expected, this issue can be closed.

[brianc]

Great! Thanks!