Report lagwarnings / timeout also while waiting for other clients to connect
ClosedPublic
Actions

Authored by elexis on Jun 1 2018, 12:04 AM.

Details

Reviewers

causative

Commits

rP21832: Report network timeouts and lag warnings to clients that finished the loading…

Trac Tickets

#5193

Summary

As reported in #5193, when the local client finished the loading screen but some other client didn't, there is the "Waiting for other clients" screen.
If said other client is actually losing the connection, it will not be seen onscreen and the waiting players are mislead to believe that the dropper is still loading.

This patch informs the host that there is likely a connection issue and that the client may be considered to be kicked rather than waiting a minute or longer while players lose patience.
This will be especially important after D1513.

Test Plan

Use this bash script to simulate lag on localhost but not eth0. Start a game with two players and call setlag 80000 to induce a long timeout.
Call setlag 0 to disable it again.

function setlag()
{
#	for device in "eth0"; do
	for device in "lo"; do
		sudo tc qdisc del dev $device root netem;
		if [ $1 -ne 0 ]; then
			sudo tc qdisc add dev $device root netem delay ${1}ms;
		fi
	done
}

Notice that the NetServer still only send the warnings to players who finished the loading screen (CNetServerWorker::CheckClientConnections).

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jun 1 2018, 12:04 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/605/display/redirect

Harbormaster completed remote builds in B6190: Diff 6690.Jun 1 2018, 12:09 AM

elexis edited the test plan for this revision. (Show Details)Jun 1 2018, 12:39 AM

elexis added inline comments.Jun 1 2018, 12:50 AM

source/network/NetClient.cpp
779 ↗	(On Diff #6690)	The NetClient still sees itself as NCS_LOADING until the "Waiting for other clients" screen disappeared, because only the transition on `NMT_LOADED_GAME` leads out of that state. Hence the condition removed here.
source/network/NetServer.cpp
557 ↗	(On Diff #6690)	`SERVER_STATE_LOADING` only left after all clients finished loading, hence the removal here.

Tested, works. The message is still being sent while the client is on the loading screen, but it's hidden there and causes no problems.

source/network/NetClient.cpp
771 ↗	(On Diff #6690)	"also" should be used instead of "too." Better to say: "This report should also be displayed while the game is waiting for clients to finish the loading screen." Actually, I think this comment is not needed. There's no need to single out the loading screen, when this code also works in game setup and in game, neither of which are mentioned.

This revision is now accepted and ready to land.Jun 1 2018, 11:59 AM

elexis mentioned this in D1513: Prevent players from dropping during map generation.Jun 1 2018, 5:47 PM

In D1546#62599, @causative wrote:

Tested, works.

Thanks for the review causative. They are hard to get for networking code!

The message is still being sent while the client is on the loading screen, but it's hidden there and causes no problems.

So I thought too at first, but it's untrue. See the last line of the testplan (which I had added after the original post):

Notice that the NetServer still only send the warnings to players who finished the loading screen (CNetServerWorker::CheckClientConnections).

So the server still sends no useless messages! (The messages were not hidden but not pulled as only the gamesetup and session page call displayGamestateNotifications in gui/common/functions_global_object.js).

source/network/NetClient.cpp
771 ↗	(On Diff #6690)	Your proposed comment is better than mine. But I disagree with the comment being unneeded. The loading screen is not singled out, the waiting-for-clients phase is. This phase is not represented in the NetSession states, so it is very easy to forget this phase. The intention is to prevent people from adding these early returns again as they may assume that the loading screen phase can be omitted (just like I did when introducing them). Actually let me move this comment to `CheckClientConnections` as that sends the message, rather than this function which only relays to the GUI.
source/network/NetServer.cpp
596 ↗	(On Diff #6690)	See this condition (or disjunction if we are looking for fancy words)