Skip to content

Throw error on connection failed in pool #708

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

Closed
Redsandro opened this issue Jan 15, 2014 · 14 comments · Fixed by #726
Closed

Throw error on connection failed in pool #708

Redsandro opened this issue Jan 15, 2014 · 14 comments · Fixed by #726
Assignees

Comments

@Redsandro
Copy link

When I var pool = mysql.createPool(options); to a wrong host (ip address), I catch nothing in pool.on('error', function(error) {});.

Instead, not until I do a pool.getConnection() + connection.query() and sit out the full (default) timeout of two minutes or something do I receive an ETIMEDOUT error.

Is there (or should there be) a way to find out immediately if the connection failed?

In contrast, if I (purposely) specify a wrong username, I immediately catch an ER_DBACCESS_DENIED_ERROR error. But a wrong host will just do nothing until the connection times out.

@dougwilson
Copy link
Member

The poll doesn't even make a connection to the database until a pool.getConnection() call, so that would be the earliest point to know.

@Redsandro
Copy link
Author

That makes sense, but then I should get an error that a connection could not be made. But it waits until a timeout is thrown in stead.

So, just like ER_DBACCESS_DENIED_ERROR is thrown when the user/pass is wrong, can't I 'catch' a wrong host instead of just having the query time out?

This is only when host is an ip (e.g. 1.1.1.1), when host is a name e.g. 'localhostiness' (deliberate typo) I get an error thrown immediately.

@Redsandro
Copy link
Author

Testcase

Connecting to a non-existing host will not throw an error, instead it will wait forever (for timeout).
I've wrapped the query inside a timeout of two seconds, otherwise this demonstration would take a few minutes.

If you fill in correct database credentials you will see that any other mistake will throw an error immediately (once the query is executed).

var mysql       = require('mysql')  // npm install mysql
  , q           = require('q')      // npm install q
  , util        = require('util')
  , queue       = []
  , clientOpts  = {
        host        : '1.1.1.1',
        user        : 'user',
        password    : 'pass',
        database    : 'mydb'
    };

// If you have a correct host in clientOpts but a wrong user/name/db, you'll receive an error immediately.


var pool    = mysql.createPool(clientOpts);

pool.on('connection', function(connection) {
    util.log('Connected to MySql db');
});

pool.on('error', function(err) {
    util.log('Oh shit.');
    throw err;
});

exports.query = query;
function query(query, values) {
    var link;

    return q.nfcall(pool.getConnection.bind(pool))
    .then(function(connection) {
        link = connection; // scope
        return q.nfcall(link.query.bind(link), query, values);
    })
    .timeout(2000, 'Big fat timeout') // Adding short timeout because the default is a few minutes
    .then(function(rows) {
        link.release();
        return rows;
    })
    .fail(function (err) {
        util.log(err);
        throw err;
    });
}


// Test the connection
this.query('SELECT 1').then(function(rows) {
    util.log(rows);
}).done();

@dougwilson
Copy link
Member

The query is not what is timing out, it is the call to pool.getConnection that times out, because it calls connection.connect() before passing you the connection. This means you only have to wait for a pool.getConnection() call to timeout; you cannot even make the connection.query() call.

var mysql       = require('mysql')  // npm install mysql
  , util        = require('util')
  , queue       = []
  , clientOpts  = {
        host        : '1.1.1.1',
        user        : 'user',
        password    : 'pass',
        database    : 'mydb'
    };

// If you have a correct host in clientOpts but a wrong user/name/db, you'll receive an error immediately.


var pool    = mysql.createPool(clientOpts);

pool.on('connection', function(connection) {
    util.log('Connected to MySql db');
});

exports.query = query;
function query(query, values, callback) {
    pool.getConnection(function(err,connection) {
        if (err) return callback(new Error('connection error: ' + err.message));
        connection.query(query, values, function(err,rows){
            connection.release();
            if (err) return callback(new Error('query error: ' + err.message));
            callback(null,rows);
        });
    });
}


// Test the connection
this.query('SELECT 1', [], function(err,rows) {
    if (err) throw err;
    util.log(rows);
});

results in

Error: connection error: connect ETIMEDOUT

@Redsandro
Copy link
Author

I am not sure I am following. Because using proper credentials, the connection and the query are established/queried without a problem.

q.nfcall(pool.getConnection.bind(pool))
    .then(function(connection) {}

is the same as

pool.getConnection(function(err,connection) {}

q is just a library for flattening those (annoying) nested callbacks that are the side-effect of asynchronous code.
If err is defined, all .then() will be skipped and.fail() executed in stead.

But since your example throws immediately, there must be something wrong on my end. I'm just not seeing the problem. I will experiment with this.

@dougwilson
Copy link
Member

When you use Q.timeout, it will timeout on the entire chain, so even though you put the timeout call after the query call, the timeout is still encompassing the pool.getConnection call, as that is the part that is timing out, not the query as your original post was saying.

@Redsandro
Copy link
Author

This is true, but one would expect .fail() to be executed immediately, because getConnection() calls back with err (and that doesn't take 2+ seconds).

@dougwilson
Copy link
Member

OK. Does the code I posted call back as fast as you are looking for? If so, then it seems like your issue would be with your use of the Q library (which I don't know much about), which is why I was posting in the standard callback-style.

@Redsandro
Copy link
Author

I will get back to you on that. 👍

@Redsandro
Copy link
Author

Sorry for the delay.

Does the code I posted call back as fast as you are looking for?

No it does not. The callback never gets executed in your code either.
If you chance host to an existing one, e.g. 127.0.0.1, you'll get an ER_ACCESS_DENIED_ERROR error immediately, just like with my code.

Looks like my initial report was valid. There is no error thrown when you are trying to connect to a non-existing host. So we need to guess based on a lack of response before a certain timeout.

@dougwilson
Copy link
Member

@Redsandro can you try using the code from PR #726 and specifying connectTimeoutin your connection/pool configuration and seeing if this causes a timeout in your situation?

var pool = mysql.createPool({
  host: '1.1.1.1',
  user: 'user',
  password: 'pass',
  database: 'mydb',
  connectTimeout: 2000 // <-- this is new, 2s from your example with Q
});

@Redsandro
Copy link
Author

@dougwilson Sorry for the late response, I was on the toilet.

Indeed I get the a timeout error when specifying this connectTimeout.

Error: connect ETIMEDOUT

Set this to a value higher than 10 seconds and a different hardcoded timeout wins:

Error: Handshake inactivity timeout

It makes sense, because it should not take that long to connect. When I opened this bug I didn't understand why a timeout was needed at all, but it makes sense. We cannot know that a connection was failed if we do not get a reply from the (non-existing) server. If we connect to an existing server with bad credentials, we get ER_ACCESS_DENIED_ERROR immediately, because the server tells us.

I'm satisfied and I forgot to let you know. 👍

So don't change anything on my account.

With that said, I do have the feeling that setting connectTimeout should change the time before a Handshake inactivity timeout occurs in stead of adding a separate timeout with a separate connect ETIMEDOUT error on top of that.

@dougwilson
Copy link
Member

Hi @Redsandro , the inactivity timeout is not hard-coded in any way. That Handshake inactivity timeout is a different timeout tracking a different aspect of the connection, and you control that using the acquireTimeout option in your pool options (all pool options are documented at https://github.com/felixge/node-mysql#pool-options).

With that said, I do have the feeling that setting connectTimeout should change the time before a Handshake inactivity timeout occurs in stead of adding a separate timeout with a separate connect ETIMEDOUT error on top of that.

These are two independent timeouts and track different aspects of the connection lifecycle. They are additive, because they are separate. The connectTimeout is a timeout when establishing the underlying TCP connection to your MySQL server. Once this succeeds and you have a TCP channel, then the timeout is destroyed. After that, this module will send a handshake packet to the MySQL server. The acquireTimeout will time this aspect of establishing a MySQL application session and the login of your user.

These are separate timeouts because they are very different in nature -- one is sensitive to your networking infrastructure and the other is sensitive to your MySQL application/authentication infrastructure. Typically you want to have a very small connectTimeout and a longer acquireTimeout (we default to 10s + 10s, which is 20s total).

I hope this helps!

@dougwilson dougwilson self-assigned this Jan 11, 2016
@Redsandro
Copy link
Author

Thank you for the elaborate response. I get it now. I have no further comments at this time. 😄

@mysqljs mysqljs deleted a comment from nirmalgoswami Jul 27, 2017
@mysqljs mysqljs deleted a comment from vurumadla Jul 27, 2017
dveeden pushed a commit to dveeden/mysql that referenced this issue Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants