Connection handling needs work inside web request handlers

In server/routes/index.js the pattern used for closing connections is incorrect given the method used to acquire the connection. It should be calling done() instead of client.end(). Calling client.end() would be correct if the connection was acquired using:

var client = new pg.Client(connectionString);
client.connect();

In this case, we really do want to open connections using pg_connect() however, because the connection pooling provides a huge boost in performance and stability.

See below for an example of the problem:

router.get('/api/v1/todos', function(req, res) {

    var results = [];

    // Get a Postgres client from the connection pool
    pg.connect(connectionString, function(err, client, done) {

        // SQL Query > Select Data
        var query = client.query("SELECT * FROM items ORDER BY id ASC;");

        // Stream results back one row at a time
        query.on('row', function(row) {
            results.push(row);
        });

        // After all data is returned, close connection and return results
        query.on('end', function() {
            client.end();
            return res.json(results);
        });

        // Handle Errors
        if(err) {
          console.log(err);
        }

    });

});

Furthermore the error handling should be moved to the top of the function and should not proceed if there is an error. An error here represents an error retrieving a database connection. Otherwise the code may proceed and try to do queries. For example:

router.get('/api/v1/todos', function(req, res) {

    var results = [];

    // Get a Postgres client from the connection pool
    pg.connect(connectionString, function(err, client, done) {
        // Handle connection errors
        if(err) {
          done();
          console.log(err);
          return res.status(500).json({ success: false, data: err});
        }

        // SQL Query > Select Data
        var query = client.query("SELECT * FROM items ORDER BY id ASC;");

        // Stream results back one row at a time
        query.on('row', function(row) {
            results.push(row);
        });

        // After all data is returned, close connection and return results
        query.on('end', function() {
            done();
            return res.json(results);
        });

    });

});

Mixing client.end() and done() inside functions like this can be dangerous. Thankfully the code as written did not mix them. However, I ran across some code patterned after this that did, and the underlying pool logic can get confused and then fail to open a database connection entirely under certain conditions.

Please expect an immanent pull request that fixes these issues.

mjhea0 / node-postgres-todo

Connection handling needs work inside web request handlers #3