ping timer bugfix, connection timeout adjustment

[sss123next]

diff --git a/src/mumble/ServerHandler.cpp b/src/mumble/ServerHandler.cpp
index 7e30e76a..d02ad0c1 100644
--- a/src/mumble/ServerHandler.cpp
+++ b/src/mumble/ServerHandler.cpp
@@ -349,10 +349,9 @@ void ServerHandler::run() {

                tTimestamp.restart();

-               // Setup ping timer;
-               QTimer *ticker = new QTimer(this);
-               connect(ticker, SIGNAL(timeout()), this, SLOT(sendPing()));
-               ticker->start(5000);
+               ping_ticker = new QTimer(this);
+               connect(ping_ticker, SIGNAL(timeout()), this, SLOT(sendPing()));
+

                g.mw->rtLast = MumbleProto::Reject_RejectType_None;

@@ -370,6 +369,8 @@ void ServerHandler::run() {
                        shouldTryNextTargetServer = false;
                }

+               ping_ticker->stop();
+
                if (qusUdp) {
                        QMutexLocker qml(&qmUdp);

@@ -384,7 +385,6 @@ void ServerHandler::run() {
                        qusUdp = NULL;
                }

-               ticker->stop();

                ConnectionPtr cptr(cConnection);
                if (cptr) {
@@ -619,7 +619,7 @@ void ServerHandler::serverConnectionStateChanged(QAbstractSocket::SocketState st
                tConnectionTimeoutTimer = new QTimer();
                connect(tConnectionTimeoutTimer, SIGNAL(timeout()), this, SLOT(serverConnectionTimeoutOnConnect()));
                tConnectionTimeoutTimer->setSingleShot(true);
-               tConnectionTimeoutTimer->start(30000);
+               tConnectionTimeoutTimer->start(120000); //this may take some time on server with large dh
        } else if (state == QAbstractSocket::ConnectedState) {
                // Start TLS handshake
                qtsSock->startClientEncryption();
@@ -651,6 +651,11 @@ void ServerHandler::serverConnectionConnected() {
                return;
        }

+       // Setup ping timer;
+       //do this after succesfull connection, not before
+       ping_ticker->start(10000); //do not waste too much traffic here, 10 seconds instead of 5 is ok
+
+
        MumbleProto::Version mpv;
        mpv.set_release(u8(QLatin1String(MUMBLE_RELEASE)));

diff --git a/src/mumble/ServerHandler.h b/src/mumble/ServerHandler.h
index bb558c4b..130e65aa 100644
--- a/src/mumble/ServerHandler.h
+++ b/src/mumble/ServerHandler.h
@@ -77,7 +77,7 @@ class ServerHandler : public QThread {
        public:
                Timer tTimestamp;
                int iInFlightTCPPings;
-               QTimer *tConnectionTimeoutTimer;
+               QTimer *tConnectionTimeoutTimer, *ping_ticker;
                QList<QSslError> qlErrors;
                QList<QSslCertificate> qscCert;
                QSslCipher qscCipher;

[mkrautz]

Thanks for the patch. Some commentary or just a few words to go along with it would be nice.

[sss123next]

it does few things.

1. increase connection timeout, it may be necessary for slow server with large dh params.
2. is more important, with current logick ping ticker starting just after connection start (before ssl handshake done ), so it expire after 5 seconds (which also make tConnectionTimeoutTimer completely useless)
3. increase ping inteval to 10 seconds instead of 5 to reduce overall network load (this questionable of course, but i think 5 is too agressive)

[mkrautz]

I think it would be neat if the connection timeout and the ping interval were settings instead. Then users could increment them themselves if necessary.

[sss123next]

of course. but at least ping time bug should be fixed asap.

[mkrautz]

@sss123next Is the change to use a 120 second connection timeout necessary on the server(s) you're testing against?

[mkrautz]

Preliminary PR for the ping timer issue at https://github.com/mumble-voip/mumble/pull/3303

[sss123next]

@mkrautz i think it is not, i have set it before discovered ping timer bug. on my servers i have ~10-20sec for 9k dh params.

[mkrautz]

Hi, we've landed a few PRs addressing this:

mumble-voip/mumble#3303 mumble-voip/mumble#3304

Is that sufficient? Or do we need to raise the ping interval and/or connection timeout duration?

[sss123next]

i think it's better to have settings. defaults should be ok for most common server/client, so i think what it's more than enough. thx for quick fix.