Timeout on connection

[crystalik]

Added timeout on connection. It's very critical for me.

[dougwilson]

How does this affect the use of the pool? Will this suddenly cause the connections that are idling in the pool to get ejected after 3 seconds? What about performing a query that takes longer than 3 seconds?

[dougwilson]

Here are my findings:

Since this sets a default timeout of 3 seconds, it seems like this is backwards-incompatible. Performing a query that takes 5 seconds (over the timeout limit):

connection.query('SELECT SLEEP(5)', function(err) {
  if (error) console.error('got error: ' + err);
  console.log('no error');
});

results in:

got error: Error: timeout

when previously with the same connection it would have succeeded just fine. It also doesn't seem like a query taking longer than the timeout should be a protocol error as implemented in their PR, but idk.

This change will in fact cause connections to get ejected from the connection pool after idling for 3 seconds. This seems like it would make the pool less useful, but it would at least be a major change in behavior from before.

What part of the process is this intending to detect a timeout in, as it seems to be triggering all over the place when there isn't an error condition.

[sidorares]

It might be useful to have connect timeout, but query timeout probably should live in user's code. Another useful timeout - "How much time command spend in the queue", sometimes you want to discard query if it spent too much time before actually sent to server

[dougwilson]

This PR adds a timeout for no network activity on the socket, which seems wrong to me, as it'll timeout when a query takes longer than the timeout (since no data is being sent/received during that time) and it'll timeout when connections sit in the pool (which is valid, but since they don't send/receive data, this kills them).

@crystalik were you intending for this to just be a timeout on connection?

[crystalik]

Sorry for my English. Try to explain.
I'm building a cluster for sphinx search nodes with realtime (RT) indexes. When one of the search nodes is unavailable, it must be immediately removed from the pool and query should go to another available server. "Long time" is 3000 ms by default, and I'm overriding it in connection options and changing it to 500 ms.

[dougwilson]

Ah. As implemented here, this timeout option does not tell you when a server becomes unavailable, rather it tells you when no data has been sent or received for 3000 ms, which kills any query that takes over 3000 ms by default. Also, since the timeout is passed to setTimeout, the way the configuration is implemented makes it impossible for people to opt out, as you need to call setTimeout(0) to disable the timeout, but the configuration will just override 0 to 3000.

[sidorares]

It should work (in theory) with current pool design: if tcp connection is closed pool connection is destroyed.
Have you tried pool or poolCluster?

[crystalik]

@dougwilson, i set the connection for each request. If the server is unavailable, it will be removed from the cluster through the time that I have (setTimeout(500)). Because the timeout triggered before the connection is established.
"but the configuration will just override 0 to 3000" - You're right, it is necessary to correct.

@sidorares, я использую poolCluster. Но в нём нет таймаутов на соединение. Если сервер недоступен, то getConnection пытается установить соединение в течение примерно 10 секунд (я не знаю откуда это время). Это слишком много для меня, так как соединение устанавливается при каждом запросе.

[sidorares]

@crystalik - а у тебя есть контроль над shinx серверами? Отчего они бывают "недоступны"? 10 секунд похоже на таймаут когда от сервера не приходит вообще ничего (т е закрыт файерволом). Eсли сервер не в сети или в сети но никто порт не слушает то обычно мгновенно приходит EHOSTUNREACH или ECONNRESET

[crystalik]

@sidorares, контроль есть, всё в нашей инфраструктуре. Они иногда падают, и надо мгновенно начать использовать другой сервер. Та реализация, которую я зареквестил, справляется. Если сервер падает и при запросе, коннект не установился в течение 500 мс, то срабатывает timeout, происходит reconnect и согласно Round Robin из пула берётся другой сервер. А этот через n попыток будет удалён из пула.

10 секунд - это когда сервера не в сети, я отключаю vpn, чтобы сымитировать падение.

Возможно, предложенная реализация далека от совершенства :-)

[sidorares]

You can have your custom getConnectionWithTimeout like this:

function getConnectionWithTimeoutAndRetries(cb) {
   var connected = false;
   var numRetries = 0;
   var maxRetries = 8;
   var timeout;
   function doConnect() {
     if (numRetries == maxRetries)
       return cb(new Error('doh'));
     pool.getConnection(function(err, conn) {
       if(err) {
           numRetries++;
           return doConnection();
       }
       if (connected) {
           connection.release();
           return;
       } else {
           connected = true;
           clearTimeout(timeout);
           cb(null, connection)
     });
   }
   function checkConnected() {
     if (!connected) {
        cb(new Error('...'))
        connected = true; // prevent callbacks later
     }
   }
   setTimeout(checkConnected, 1000);
   doConnect();
}

[sidorares]

@crystalik можно попробовать пинговать (через connection.ping()) раз в секунду и если ответа нет то выкидывать соединение. setTimeout на сам коннекшн плох по уже указаным причинам - будет закрываться соединение при живом сервере и законных 'SELECT SLEEP(3.5)' ( или просто при падении нагрузки )

[crystalik]

@sidorares, да, такое решение пробовал. Может до ума не довёл, но получалось так, когда checkConnected() вызывал cb(new Error('...')), то pool.getConnection(...) в doConnect(), продолжало висеть и ждать те самые 10 секунд.
Ну и хотелось нормального решения на уровне драйвера mysql.

[sidorares]

на уровне драйвера будет maxWait тут - #505

[sidorares]

ну и пускай висит - тебе же уже каллбэк сказал про ошибку?

[crystalik]

Да, сказал. Попробую давести до ума такое решение. Благодарю за дискуссию.

[sidorares]

не за что :)

[mikermcneil]

These guys say the config should live in the db itself. Of course, if you have multiple apps connecting to the same MySQL db, and you don't want to change the settings for your other apps, this doesn't help you.

@felixge Is this something you'd like to see added to node-mysql? (happy to send a pull request for it, should be pretty trivial) Otherwise I can just implement a wrapper on the sails-mysql side. (and ps- great library, thanks!)

@sidorares @crystalik lost track of what was going on in the conversation there :) Thanks for the help everyone!

[felixge]

@mikermcneil I've not fully followed this discussion, but I think you're asking about adding a method that returns a connection that has a server side query timeout configured? I'd be -1 on that as it seems very high level.

[mikermcneil]

Almost- I mean a setting when connecting that sets a timer for a connection timeout, eg if it takes longer than 30 seconds, callback triggered with error. Very easy to implement, and I can put it in the Sails adapter, but I'm just checking to see if you think it belongs here first :)

Mike's phone

On Feb 3, 2014, at 1:35, Felix Geisendörfer [email protected] wrote:

@mikermcneil I've not fully followed this discussion, but I think you're asking about adding a method that returns a connection that has a server side query timeout configured? I'd be -1 on that as it seems very high level.

—
Reply to this email directly or view it on GitHub.

[dougwilson]

@mikermcneil does PR #726 implement the timeout that you are looking for?

[mikermcneil]

@dougwilson I believe so sir, thank you!