brianc / node-pg-pool

A connection pool for node-postgres
MIT License
180 stars 64 forks source link

Unhandled promise rejection #76

Closed phongyu closed 6 years ago

phongyu commented 6 years ago

Hi, I am using pool in express node js for my RestAPI

var pool = new Pool(config)
function listDetails(req, res) {
  pool.connect().then(client => {
        client.query(sql_str, 
            [userId])
            .then(ressult => {
          client.release()

          var resp = new Object();
          var jsonArr = []; // Populate the result

          var data = result.rows;

          for (var i = 0; i < data.length; i++) {
              var jsonObj = new Object();
              jsonObj.game_id = data[i].game_id;
              jsonObj.record_id = data[i].record_id;

              jsonArr.push(jsonObj);
          }

          resp.details = jsonArr;

          res.json(resp);
        })
        .catch(e => {
          client.release()
          console.error('query error', e.message, e.stack)
        })
      })

I receive the error :

(node:19391) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): Error: Release called on client which has already been released to the pool.

Any suggestion is appreciated

sehrope commented 6 years ago

This is a bug in your code. You're calling release twice. Once in the catch() and once in the then().

The order of operations that lead to the issue is:

  1. Execute query
  2. Call then() handler with result
  3. Call client.release() in then()
  4. Typo in "ressult" (two "s") vs "result" likely causing an error on the line data = result.rows for accessing a variable that does not exist.
  5. The catch() gets executed which tries to call release() again
  6. This throws a new error
  7. No second catch() handler so the error bubbles up to the top and gives an unhandled rejection warning.

Get rid of the manual connection management and use pool.query(). It handles properly acquiring and releasing resources.

phongyu commented 6 years ago

Thank you so much, I switch to pool.query(), and it works perfectly. It should be the best practices. I just wonder why document explains too much about the manual connection management where is very risky in releasing resources.

brianc commented 6 years ago

@phongyu you gotta check out connections, use them, and then release them manually whenever you're doing transactions so I thought it was important to cover that since using transactions is pretty common. I'll try to do a better job of pointing out that pool.query is recommended when you're not doing transactions. Thanks for the feedback! and @sehrope thanks for chiming in on this so quickly.