javascript promises for pooling connections and executing query

Question

I'm wondering if this approach is correct or does it need further refinements, maybe promisify custom mySQL getConnection method as well???

    request: function(queryRequest) {
        return new Promise(function(resolve, reject){
            Database.getConnection(function(error, connection){
                if(error) {
                    reject({error: error, queryRequest: queryRequest});
                } else {
                    connection.query(queryRequest.sql, queryRequest.values, function(error, rows, fields){
                        if(error) {
                            reject({error: error, queryRequest: queryRequest});
                        } else {
                            resolve({rows: rows, fields: fields, queryRequest: queryRequest});
                        }
                    });
                }
            });
        });
    },

The getConnection method defined in Database module.

    getConnection: function(callback) {
        this.pool.getConnection(function(error, connection){
            callback(error, connection);
        });
    },

Ah, you fixed `reject(error, queryRequest)` before I could comment :-) — Bergi, Jun 29 '15 at 13:52
I recommend a [disposer pattern](http://stackoverflow.com/questions/28915677/what-is-the-promise-disposer-pattern) with connection pooling or a built in facility of the promise library (`using` in bluebird) — Benjamin Gruenbaum, Jun 29 '15 at 13:54
[`node-mysql-promise`](https://github.com/martinj/node-mysql-promise) provides a promisified version of `node-mysql`. — robertklep, Jun 29 '15 at 13:54
That said, this is opinion based, so I'm voting to close, you're welcome to come discuss this in the javascript chat here or at #promises on IRC. — Benjamin Gruenbaum, Jun 29 '15 at 13:55
I'm still trying to get it right... :) since there are two async operations involved with executing any query, 1. Pooling Connection, 2. Executing Query, I wonder if one promise is enough or should there be another promise nested inside the query executor module? I'm using native Javascript promise and don't want to use Q library etc. — user2727195, Jun 29 '15 at 13:55
@robertklep I looked at node-mysql-promise but I don't see any code that will reject in case database connection or pooling failed. — user2727195, Jun 29 '15 at 13:57
@user2727195 you're right, it doesn't provide promisified pool operations :( — robertklep, Jun 29 '15 at 13:58

score 2 · Accepted Answer · edited May 23 '17 at 12:09

2

maybe promisify custom mySQL getConnection method as well?

Only maybe. While it could be considered a bit cleaner, and makes your callback pyramid a bit flatter, it doesn't improve the code much:

function request(queryRequest) {
    return new Promise(function(resolve, reject) {
        Database.getConnection(function(error, connection) {
            if (error)
                reject(error);
            else
                resolve(connection);
        });
    }).then(function(connection) {
        var res = new Promise(function(resolve, reject) {
            connection.query(queryRequest.sql, queryRequest.values, function(error, rows, fields) {
                if (error)
                    reject(error);
                else
                    resolve({rows: rows, fields: fields, queryRequest: queryRequest});
            });
        });
        return res;
    }).catch(function(error) {
        throw {error: error, queryRequest: queryRequest};
    });
}

I'm wondering if this approach is correct

For database connections, you might want to have a look at the disposer pattern. If you don't need it, you still should remember to always release your connections, using something like

….then(function (connection) {
    var res = …;
    // better be solved using `finally` where supported
    return res.then(end, end);
    function end() {
        connection.release();
        return res;
    }
})

Also, rejecting promises with objects that are no Errors is a bad practise, you might better do

….catch(function(error) {
    error.queryRequest = queryRequest;
    throw error;
})

or the equivalent in your original pattern.

edited May 23 '17 at 12:09

Community

1
1

answered Jun 29 '15 at 14:06

Bergi

630,263
148
957
1,375

there's some semicolon duplicates in the second then, please review. I read about disposer pattern, could we use `connection.release()` in the second resolve? +1 for the `error.queryRequest` suggestion – user2727195 Jun 29 '15 at 14:18
What do you mean by "semicolon duplicates"? There's no syntax error. – Bergi Jun 29 '15 at 14:21
sorry about that, it's correct. do you suggest using `connection.release` under second resolve in view of disposer pattern? – user2727195 Jun 29 '15 at 14:24
Yes, you probably should include `connection.release` in this `request` method. Maybe in an extra `then` handler, especially if it can throw. – Bergi Jun 29 '15 at 14:27
The disposer pattern is especially useful if you are doing different things with the `connection` every time. Is `connection.query` the only thing you use, or do you have other methods that call `Database.getConnection` as well? – Bergi Jun 29 '15 at 14:28
an extra then would be elegant, can you please add this to your answer. To answer your comment, this connection is not used again, it should return the connection back to the pool. if another request comes in, a new instance is created that pools the connection and executes the queryRequest – user2727195 Jun 29 '15 at 14:28
No, I didn't mean whether the same `connection` object was reused, but whether you have different methods that are structurally similar to `request`, but calling other methods than `.query` on their respective connections? In that case, the disposer pattern would be advisable. – Bergi Jun 29 '15 at 15:31
could be an alternate approach, for instance `getConnection` returning a promise, and then the module that called the `getConnection` is calling `then` on that, and it `rejects` and `resolves` for it's caller. To answer your last comment, I've no other methods, this module's only purpose is to pool the connection and execute query on – user2727195 Jun 29 '15 at 15:31
I can ask another question if you prefer for a refactored version – user2727195 Jun 29 '15 at 15:38
@user2727195: Sure, go for it! – Bergi Jun 29 '15 at 16:08

javascript promises for pooling connections and executing query

1 Answers1

Linked