pg-pool swallows errors

[cressie176]

I'm seeing strange behaviour when using pg-pool. The following script demonstrates the problem

const Pool = require('pg-pool')
const pool = new Pool()

pool.connect((err, client, close) => {
  client.query('SELECT 1', () => {
    close()
    pool.connect((err, client, close) => {
      throw new Error()
    })
  })
})

setInterval(() => {}, Number.MAX_VALUE)

Instead of crashing, the error is swallowed. I noticed this because my code was programatically building a query, and thew an error because of an undefined variable. e.g.

const Pool = require('pg-pool')
const pool = new Pool()

pool.connect((err, client, close) => {
  client.query('SELECT 1', () => {
    close()
    pool.connect((err, client, close) => {
      const statement = getStatment() // Typo meant getStatment was not defined
      client.query(statement, (err, result) => {
        console.log(result)
      })
    })
  })
})

[cressie176]

Is this project still maintained?

[brianc]

Yes.

On Wednesday, August 24, 2016, Stephen Cresswell notifications@github.com
wrote:

Is this project still maintained?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#24 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADDoS2XDKi7ZKjETi5WebLB4oY1pDQRks5qjCtdgaJpZM4JiNaN
.

[iamchenxin]

@cressie176 This caused by promise's default behavior.(Promise will do no thing, when you did not catch a reject).
You could listen the event unhandledrejection to change the default behavior.

process.on('unhandledRejection', (reason, promise) => {
  console.log('Unhandled Rejection at: Promise', promise, 'reason:', reason);
  // application specific logging, throwing an error, or other logic here
});

unhandledrejection will listen all unhandled rejection of promise, So you can throw in inner then crash without catch them ( I think this problem is related to node-postgres/query.js. Maybe you can amend the source code, and separate callback from this behavior).

[cressie176]

Hi @iamchenxin. Thanks for the response. I'm using the callback API so not able to catch a reject. Also listening process.on('unhandledRejection') isn't going to help much since that could be caused by any unhandled rejection, not specifically one raised by the pg internals.

I'd definitely argue that this is a bug with pg (or one of its libs) since users of the callback API shouldn't need to understand the libraries internals. By convention any errors should be passed the callback via the err argument.

[iamchenxin]

@cressie176 look it again, seems this is not only belong to node-postgres/query.js. all functions which using Promise inner keep the Promise's default behavior too.(ex: pool.connect ). (Test with nodejs 6.31, but im not sure to this, a little sleepy)
Maybe you could simply pass a bluebird Promise to the pool's config, so the global Promise will keep the origin behavior (The bluebird's default behavior will raise a error when not catch a reject).

Or You could temporaryily chain a catch below your original function.

    pool.connect((err, client, close) => {
      throw new Error()
    }).catch( e => { // catch it here...

    })

I agree with you, and also think may be the callback's behavior should be separated from promise in source code.
or Alternatively maybe should test the bluebird Promise and set it to the default promise for pg in source code?

[cressie176]

Thanks again @iamchenxin. Both those options (bluebird and catch) work with my simplified example, but unfortunately aren't viable options with the real code.

The problem is that the Bluebird intercepts unhandled rejection and logs it, but there's still no way (I've been able to find) to do anything intelligent with the error.

catching the error after each call does appear to work, but doing that renders the callback API worse than pointless for two reasons

1. The catch is likely to intercept any error thrown by end user code, and not just database related ones
2. The likely response to both a callback err and caught err will be to return cb(err). It's possible in this circumstance to execute cb(err) twice, causing all sorts of problems with any code that uses async.

[iamchenxin]

@cressie176
Below is how pg-pool works, currently callback is wrapped in Promise(so the Exception throwed in callback will be handled by Promise). If you need a raw callback's behavior, you must rewrite this code, (build a callback layer, then a Promise layer above it.). Seems there is no simply way to produce the raw callback's behavior, except rewriting. Maybe you can ask @brianc about this.

Pool.prototype.connect = function (cb) {
  return new this.Promise(function (resolve, reject) {
    this.log('acquire client begin')
    this.pool.acquire(function (err, client) {
      if (err) {
        this.log('acquire client. error:', err)
        if (cb) {
          cb(err, null, function () {})
        }
        return reject(err)
      }

      ........

      if (cb) {
        cb(null, client, client.release)
      }

      return resolve(client)
    }.bind(this))
  }.bind(this))
}

[cressie176]

Thanks for your time and explanation @iamchenxin . Unless @brianc has any good suggestions I don't think I'll just have to live with the unhandledRejection approach.

[brianc]

@cressie176 sorry I've been a bit AWOL - I get busy & things fall by the wayside. I definitely want to fix this - having errors disappear without warning is a pretty big problem. I'll move it to the top of my queue and get to it soon. I really appreciate the example you submitted as a clear case for how to reproduce the issue. I'll ping you when I put the pull request.

@iamchenxin thanks for the investigation you did! It may indeed require a bit of rewriting - I'd like to end up at a place where the functionality can be maintained. If there is no way to do it in a backwards compatible way it may take a bit longer. In the next major version of pg we want to ship node-postgres core without promises, pg-pool without promises, and ship *-as-promised versions or an additionaly thing to require in which can allow people to seamlessly "opt-in" to promises. I think that's how we might want to go anyway - still discussing it with @joskuijpers and have been super busy with travel & family stuff so haven't had a lot of time. Feedback is always welcome!

[cressie176]

Thanks @brianc, that's perfect. I like the idea of shipping a promise free postgres-core, with an optional as promised wrapper. No need to apologise for being AWOL, I really appreciate the effort you and the other contributors have put in node-postgres.

[shakaIsReal]

+1 here's my situation pool.query(select * from userz, function(err, reply) { // ... })
with this code i still get Possibly Unhandled Rejection at: ... But changing it to client.query() works. The only way to catch the promise to use catch(err => ... )

[TechupBusiness]

UPDATE:
The issue is NOT related to node-pg and pools... its simply because I put somewhere an await and tried to use the return for another function (in this case)

return await this.select(data).rows

Trying to access a property/method of the return object of an async/await execution, seems to silently break the await and it is automatically converted to a normal async call.
https://medium.com/@techupbusiness/node-js-await-async-trap-when-chaining-2481861a4c54

---

I have also a very very weird behavior... never had this before. Sounds slightly like this issue:

I can use the pool.connect() a few times... but suddenly when creating a client it just exits the function while debugging... I tried to get any error but it seems to be impossible to get any information:

            if(debug===true) {
                this._pool.connect().then(client=> { // is not exiting the further interpreter flow
                    console.log('whoot?') // never shown or breakpoint hit
                }).catch(ex=> {
                    console.log(ex.message) // never shown or breakpoint hit
                })
            } else {
                try {
                    client = await this._pool.connect() // immediately exits the current interpreter flow and method hard 
                } catch(ex) {
                    console.log(ex.message) // never shown or breakpoint hit
                } finally {
                    console.log('WHY?') // never shown or breakpoint hit
                }
            }

catch/finally does never execute... and there is no message on the console, breakpoints doesnt work either. I also activated

        process.on('unhandledRejection', (err) => {
            console.error(err)
            process.exit(1)
        })

Also this is useless for this issue:

        this._pool.on('error', (err, client) => {
            let { logger } = self.loadModules()
            logger.logCritical(err.message, 'Database error', err)
        })

But it doesnt help. As soon as the interpreter (via debugging) hits the connect(), the surrounding method returns undefined regardless of any try/catch/finally around, or callback used to get some information. Has anyone a suggestion?