New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection handling needs work inside web request handlers #3

Closed
obscurerichard opened this Issue Sep 14, 2015 · 1 comment

Comments

Projects
None yet
1 participant
@obscurerichard
Contributor

obscurerichard commented Sep 14, 2015

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 added a commit that referenced this issue Oct 5, 2015

Merge pull request #4 from obscurerichard/pg-fixes
Fix for issue #3: refactoring of connection pool handing
@obscurerichard

This comment has been minimized.

Show comment
Hide comment
@obscurerichard

obscurerichard Oct 16, 2015

Contributor

@mjhea0 Thank you for merging pull request #4! This issue completely ate one of my weekends this September, I hope this helps others avoid a similar situation.

Contributor

obscurerichard commented Oct 16, 2015

@mjhea0 Thank you for merging pull request #4! This issue completely ate one of my weekends this September, I hope this helps others avoid a similar situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment