Fix iInFlightTCPPings check

[Profpatsch]

The iInFlightTCPPings >= g.s.iMaxInFlightTCPPings check would mean
that if 1 is in flight and another is started, it immediately
disconnects, so as OP said in
#3448 we want to have >
here.

Maybe that already fixes most of the spurious disconnects in practice,
or maybe we want to increase the default to 3 (right now it’s
effectively 2−1, so 1).

Fixes: #3448

[Krzmbrzl]

Okay I just made myself (more or less) familiar with how the system seems to work.

The value of iInFlightTCPPings is checked at

mumble/src/mumble/ServerHandler.cpp

Lines 507 to 510 in 1bd7839

	if (g.s.iMaxInFlightTCPPings >= 0 && iInFlightTCPPings >= g.s.iMaxInFlightTCPPings) {
	serverConnectionClosed(QAbstractSocket::UnknownSocketError, tr("Server is not responding to TCP pings"));
	return;
	}

which is in the same function that'll also send a new ping. This means that if the maximum is reached (iInFlighttcppings == iMaxInFlightTCPPings), the connection should be terminated as otherwise it'd send another ping and via

mumble/src/mumble/ServerHandler.cpp

Line 547 in 1bd7839

iInFlightTCPPings += 1;

iInFlightTCPPings would be increased by one and thus exceeding the maximum value. Thus the current implementation seems to be correct.

However when looking at the documentation of iMaxInFlightTCPPings at

mumble/src/mumble/Settings.h

Lines 357 to 367 in ae02608

	/// iMaxInFlightTCPPings specifies the maximum
	/// number of ping messages that the client has
	/// sent, but not yet recieved a response for
	/// from the server. This value is checked when
	/// the client sends its next ping message. If
	/// the maximum is reached, the connection will
	/// be closed.
	/// If this setting is assigned a value of 0 or
	/// a negative number, the TCP ping check is
	/// disabled.
	int iMaxInFlightTCPPings;

I agree with @radistmorse that the checking should actually be

if (g.s.iMaxInFlightTCPPings > 0 && iInFlightTCPPings >= g.s.iMaxInFlightTCPPings) {

(> instead of >= for checking the setting) as otherwise the TCP ping check would be done also if iMaxInFlightTCPPings == 0 which contradicts its documentation.
Thus this should be changed accordingly.

To address your initial concern, I think you can safely increase the default value for iMaxInFlightTCPPings by one (effectively having the same effect as the current state of this PR).

[Krzmbrzl]

@Profpatsch what's the status? Will you change the PR accordingly or should someone else overtake that?

[Profpatsch]

I don’t understand your comment fully, so please go ahead and push the changes onto the PR. I’m getting regular disconnects on perfectly fine connections here, so I’d just like it solved.

I already increased the value by 1 in my config and it still disconnects sometimes, so I wonder if the default shouldn’t be 3 or even 5.

How often are these pings sent, aka how much timeout would each increase of the option cause?

[Krzmbrzl]

How often are these pings sent, aka how much timeout would each increase of the option cause?

In the default configuration they are sent every 5 seconds.

[Krzmbrzl]

I added the respective changes and increased the allowed in-flight ping count to 4