Skip to content
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

MySQL is not reconnecting gracefully, causing store to fail with "Cannot enqueue Query after fatal error" #18

Closed
Domiii opened this issue May 30, 2015 · 8 comments

Comments

@Domiii
Copy link

Domiii commented May 30, 2015

Currently, express-mysql-session cannot properly handle DB connection failures. After a connection has been terminated, the store will always fail with:

Error during request (500): Error: Cannot enqueue Query after fatal error.
at Protocol._validateEnqueue (MY_CODE\node_modules\express-mysql-session\node_modules\mysql-connection-manager\node_modules\mysql\lib\protocol\Protocol.js:193:16)
at Protocol._enqueue (MY_CODE\node_modules\express-mysql-session\node_modules\mysql-connection-manager\node_modules\mysql\lib\protocol\Protocol.js:129:13)
at Connection.query (MY_CODE\node_modules\express-mysql-session\node_modules\mysql-connection-manager\node_modules\mysql\lib\Connection.js:185:25)
at SessionStore.get (MY_CODE\node_modules\express-mysql-session\lib\index.js:127:18)
at Layer.session [as handle] (MY_CODE\node_modules\express-session\index.js:404:11)

That is actually not a bug but intended behavior. I quote node-mysql's official documentation:

Re-connecting a connection is done by establishing a new connection. Once terminated, an existing connection object cannot be re-connected by design.

However, gracefully handling connection failures are still an open issue.

In version 0.0.12, express-mysql-session had a reconnect() method. I would suggest to 1) either re-introduce that behavior or 2) provide some other way of handling DB connection problems.

For reference, I am sharing the code that handled connection errors in 0.0.12, based on this suggestion (it works very well, and I will now go back to 0.0.12 for that reason):

// remember if connection already failed (since mysql lib might raise errors more than once)
var fataled = false;
var reconnectTimer = null;

// create store
var store = ...;

// use MySQL connection error handler hack
// see: https://github.com/felixge/node-mysql/issues/832/#issuecomment-44478560
var del = store.connection._protocol._delegateError;
store.connection._protocol._delegateError = function(err, sequence){
    if (fataled) return;        // ignore silently

    if (err.fatal) {
        // fatal error -> Gotta re-connect!
        fataled = true; 
        var reconnectDelay = dbOptions.reconnectDelay || 5;

        console.error('mysql fatal error: ' + err.stack);
        console.warn('Connection to database lost. Reconnecting in ' + reconnectDelay + 's...');

        // re-connect after delay
        reconnectTimer = setTimeout(function() {
            fataled = false;
            reconnectTimer = null;
            store.reconnect();
        }, reconnectDelay * 1000);
    }
    else {
        // propagate error
        return del.call(store.connection._protocol, err, sequence);
    }
};

// use `store`...
var session = { ... store: store, ... };
@chill117
Copy link
Owner

chill117 commented Jun 4, 2015

The reason the reconnect method (and other related methods) were removed from express-mysql-session was because that behavior is now handled by mysql-connection-manager.

Hopefully this weekend I will have some time to look into this in more detail.

@Domiii
Copy link
Author

Domiii commented Jun 4, 2015

Ok. FYI: Even if the manager re-connects, express-mysql-session is never getting the new connection from the manager.

It appears, the simplest solution is to just use a connection pool and call getConnection for each query. This way, you also do not have to implement re-sending of previously failed queries after re-connect.

@chill117
Copy link
Owner

chill117 commented Jun 4, 2015

It appears, the simplest solution is to just use a connection pool and call getConnection for each query. This way, you also do not have to implement re-sending of previously failed queries after re-connect.

You can use connection pooling by passing the following option when instantiating the session store:

{
    useConnectionPooling: true
}

@Domiii
Copy link
Author

Domiii commented Jun 5, 2015

Yeah, that's a fine option! However, you are currently caching the connection of the pool. As I said: You need to use getConnection instead to make it work.

@chill117
Copy link
Owner

chill117 commented Jun 5, 2015

Ah yes. You are correct. I will fix this over the weekend.

Well actually.. Correct me if I am wrong, but it appears that according to the node-mysql documentation which you just linked to, you can use the pool object in the same way that you would use the connection object:

var mysql = require('mysql');
var pool  = mysql.createPool({
  connectionLimit : 10,
  host            : 'example.org',
  user            : 'bob',
  password        : 'secret'
});

pool.query('SELECT 1 + 1 AS solution', function(err, rows, fields) {
  if (err) throw err;

  console.log('The solution is: ', rows[0].solution);
});

@Domiii
Copy link
Author

Domiii commented Jun 6, 2015

Hum... Interesting! So, in index.js, renaming connection to queryInterface and then either assign the connection or pool to it should fix everything.

@chill117
Copy link
Owner

chill117 commented Jun 6, 2015

No. The connection object from mysql-connection-manager is the connection pool object from node-mysql if you use the following option with the express-mysql-session store:

{
    useConnectionPooling: true
}

No modification is required to make it work even now.

@Domiii
Copy link
Author

Domiii commented Jun 7, 2015

Ok... Maybe the naming should be adjusted correspondingly?

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

No branches or pull requests

2 participants