mumble-voip/mumble

Automatic Pings do not seem to respect connecting and error states

Open

#2,627 opened on 2016年11月7日

GitHub で見る
 (0 comments) (0 reactions) (0 assignees)C++ (5,642 stars) (1,062 forks)batch import
Hacktoberfestclientfeature-requestgood first issuepriority/P3 - Somewhat important

説明

As per https://github.com/mumble-voip/mumble/pull/2622#issuecomment-258711918

I had Mumble open in a VM, trying to connect to an IP that no longer hosted a server. I got the usual errors for a while. I don't remember what it was: unable to connect?

Then, after 5-6 of them, I began getting "unable to send TCP pings to the server". I guess we start pinging before we should?

ServerHandler::run() sets up the recurring ping timer and immediately starts it. It does so after setting up the socket and initiating the connect (qtsSock->connectToHostEncrypted(qsHostName, usPort)), but without any adequate handling of no-success cases. The timer is local to the run() function; it is never paused on connection issues. So yes, we do start pinging and continue to ping in inadequate cases, namely before a server connection has been established.

Not sure where the (old) reconnect code is though, where the server automatically reconnects.

コントリビューターガイド

技術スタック
cpp
領域
desktop
Issue 種別
bug
難度新しい貢献者にとっての実装難度です。1 は非常に小さな変更、5 は専門的な作業です。
4
推定時間経験ある貢献者が調査、実装、テスト、pull request 準備にかけるおおよその時間範囲です。
half day
活動状況Issue が今どれだけ取り組みやすいかを示します。新しい、活発、古い、ブロック中、またはメンテナー入力待ちなどです。
stale
明確さ期待される変更、受け入れ条件、次の手順がどれだけ明確かを示します。
mostly clear
前提条件
C++Qt frameworkTCP networking
初心者向け度初回貢献者にどれだけ取り組みやすいかを 1-100 で推定したスコアです。
30
調査方針
Investigate ServerHandler::run() in the Mumble source code, specifically the setup of the recurring ping timer. The issue mentions that the timer is started before the connection is fully established, leading to pings being sent incorrectly during connecting or error states. Look at the referenced pull request #2622 for additional context. Potential fix involves pausing or not starting the ping timer until the server connection is confirmed.