pool.getConnection(function (error, connection) not returning a connection

[dannybisby]

Hi,

Lately i've been experiencing an issue where pool.getConnection(function (error, connection) is not returning a function and is hanging any Express.js request i have that contains MySQL queries.

I've done debugging and tracked it back to this code.

The code i'm using is

var mysql = require('mysql'), sApp = null;

module.exports = {

connect : function (app, callback) {

    sApp = app;
    pool  = mysql.createPool({

        host     : app.config.mysql.hostname,
        user     : app.config.mysql.username,
        password : app.config.mysql.password,
        database : app.config.mysql.database,           
    });

    app.logging.info("Creating a new MySQL Pool");
    callback();
},

query : function (string, params, callback) {

    pool.getConnection(function (error, connection) {

        console.log(error);

        connection.query(string, params, function (error, rows) {

            connection.end();
            callback(error, rows);
        }); 
    });
}

}

[dresende]

Do you have an error stack? What version are you using?

[dannybisby]

I was logging errors on the pool.getconnection but the callback is never
called so I can't get an error from the latest where the issue is all
errors before that return null. I was using npm latest but upgraded to git
latest to test the latest commit issue still persists

On Tuesday, March 5, 2013, Diogo Resende wrote:

Do you have an error stack? What version are you using?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/405#issuecomment-14468493
.

[dresende]

Are you sure this isn't a bug of yours? Like for example calling .query("...", callback) (without params) ?

Hi!
I have a similar problem. If I run my app for a while then all of a sudden 50% of the time I request a connection from the pool nothing happens and the app is left hanging. I wonder if it might be one or a few of the connections in the pool that are left in a state where they are not operational any longer. I run 2.0.0-alpha7.

EDIT: It seems that I did not properly clean up a query that failed because of inserting a duplicate value in the table. Now I have everything working again.

[RyanMarcus]

It appears this error can occur because of forgetting to end a query retrieved from a pool. To be sure, you can set the waitForConnections property of a pool to false. This will cause a connection request to fail if there are no more connections rather than seemingly never invoking the callback.

To figure out where I'd forgotten to end a connection, I found adding this code to the first line of Pool.prototype.getConnection to be useful:

console.log("**** Opening connection: " + arguments.callee.caller.name);

I couldn't figure out a way to make a message print out whenever I ended a connection, however, despite trying several locations in Pool and Connection. That would allow one to pinpoint, virtually instantly, the leak.

Perhaps the library could use some kind of reference counting / garbage collection to produce an error when a connection object is not properly .end'd?

[statico]

+1 to @RyanMarcus suggestion. Even with the debug and trace options enabled, there was no signal that missing a .release() was the underlying problem.

[dougwilson]

Perhaps the library could use some kind of reference counting / garbage collection to produce an error when a connection object is not properly .end'd?

How would one know when a connection should have been ended but wasn't? JavaScript does not expose a way to to when some code stops referencing a variable, and because the node.js environment is async, we cannot know when some code just forgot to call end on the connection or it is keeping it on purpose. If you send a PR that implements this somehow, that would be welcome.

[sidorares]

It's possible with https://github.com/TooTallNate/node-weak but I'm not sure that adding binary dependency worth here. IMO all pool & cluster pool code should be external to node-mysql ( I would happily reuse it in node-mysql2) and it should be possible to use custom pool like "automatic garbage collected node-weak powered" instead of default one

[statico]

@dougwilson Nobody's asking to do the impossible, but merely to improve the API to prevent programmer error.

[dougwilson]

Sure, @statico , but I saw you seconded the reference counting. Were you meaning to second a different method? I'm certainly willing to improve it, but we just can't do reference counting. What method were you thinking of? Would using or adding some additional debug flag that printing when a connection was acquired and when a connection was released work (so you can try to guess it from the logs)? The main downside to this is of course you have to suspect this is the case in order to enable the flag in the first place...

[dougwilson]

P.S. I like @sidorares suggestion of separating the pool from the main library. People could just use https://github.com/coopernurse/node-pool/ or potentially another pool that depended on a native module like https://github.com/TooTallNate/node-weak for reference counting (but we can't in the main library, because typically no Windows user can install native modules).

[statico]

Since the unexpected behavior here is caused when the pool is exhausted, I would have liked to have seen some notification as to when that happened. I'd like to see a lot more logging from this module in general (e.g., when a connection is dropped and reconnects), even if that logging were hidden behind a debug or verbose flag.

[sidorares]

I would actually suggest separation of query pre-processing as well ( with current implementations of pool & sqlstring require'd by default from external packages, no new behavior here and no extra setup steps ). Half of issues here are not related to core driver and sqlstring or pool related

[dougwilson]

I would have liked to have seen some notification as to when that happened.

This would of course have to be hidden behind some kind of flag or maybe raise an event on the pool object (we don't want to write out to stdout/err randomly).

I would actually suggest separation of query pre-processing as well

I like this idea. This is what we did to connect/express in expressjs, which is pretty sweet.