Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#3847 closed enhancement (fixed)

[PATCH] Update Game Stanzas sent too frequently

Reported by: scythetwirler Owned by: sbirmi
Priority: Should Have Milestone: Alpha 22
Component: Multiplayer lobby Keywords: patch
Cc: elexis Patch:

Description (last modified by elexis)

In gamesetup.js, handleInitPlayerAssignments is called every time a player's assignment is changed, and naively sends a new game update stanza to the lobby bot, resulted in unnecessary traffic.

The host should only report an update with joinings and leavings.

Attachments (2)

deferred-gamesettings_rev1.patch (1.6 KB ) - added by sbirmi 7 years ago.
Deferred sending of game attributes to the lobbybot
deferred-gamesettings_rev2.patch (2.2 KB ) - added by sbirmi 7 years ago.
rev1 description + name in programming.json

Download all attachments as: .zip

Change History (11)

comment:1 by elexis, 8 years ago

In 17943:

Reduce the number of lobby gamelist updates by roughly 60-80% by eliminating duplicate packets, refs #3847.

comment:2 by elexis, 8 years ago

Component: UI & SimulationMultiplayer lobby
Keywords: simple added
Milestone: Alpha 20Backlog
Type: defectenhancement

How it was improved: Before the commit above, the gamestanza would be sent thrice everytime any setting changes. So reducing the stanza spam is guaranteed to be reduced by 67%.

Furthermore most setting changes didn't change any data in the stanza at all, for example ceasefire, disabled treasures, team numbers, player civs etc., these duplicate calls were eliminated as well.

Keeping it universal: #3476 proposes to show more information in the lobby, for example team composition. Having the logic to omit the sending of packets inside the sender-function (instead of in the places that call that function) makes this spamfilter more universal and won't need to be changed in that ticket for example.

Remaining (simple) TODO: It is still possible to further reduce the spam by only sending a gamestanza in case the settings were not changed since the last 3 seconds. As suggested by scythetwirler, they should be sent immediately in case the server becomes joinable or not joinable anymore (so players are incentivized to join/not join immediately). Take a look at g_ChatTimers in messages.js for an example of how timers are implemented.

comment:3 by sbirmi, 7 years ago

Owner: set to sbirmi

Taking a whack at it. Should be simple enough. I will look around (or ask) to see what it means for the server to become joinable or not joinable.

by sbirmi, 7 years ago

Deferred sending of game attributes to the lobbybot

comment:4 by sbirmi, 7 years ago

I have a fix.. inviting feedback. Changes include:

  1. sendRegisterGameStanza() doesn't send attributes now but instead simply (re-)arms a timer (default timeout = 3 seconds). callback = sendRegisterGameStanzaImmediate()
  2. In sendRegisterGameStanzaImmediate(), we now immediately flush game attributes (and disarm the timer).
  3. When hosting a new game, we immediately flush the attributes at the end of initGUIObjects. I *think* that is the right place to immediately notify lobbybot of the new game.

For (3), manually tested (connected to lobby with two usernames). Without that one line of change, the hosted game appears 3 seconds later. With the change, it appears ~immediately.

If I cancel hosting the game, the game disappears immediately from the gamelist in the lobby.

comment:5 by sbirmi, 7 years ago

Keywords: rfc patch added
Milestone: BacklogWork In Progress
Summary: Update Game Stanzas sent too frequently[PATCH] Update Game Stanzas sent too frequently

by sbirmi, 7 years ago

rev1 description + name in programming.json

comment:6 by elexis, 7 years ago

Description: modified (diff)

Similar proposal in https://code.wildfiregames.com/P39 which also adds the change to the lobby bot but doesn't update instantly if a player joined/left (which should be done for better user feedback) which this patch appears to do

comment:7 by elexis, 7 years ago

Keywords: simple rfc removed
Milestone: Work In ProgressAlpha 22

Missing immediate update on gamestart (Bug): The lobby bot must have been updated with the most recent gamesettings before launching a game. If there was a timer running, it can be deleted and a sendRegisterGameStanzaImmediate call must be executed. Because session.xml reloads timer.js, the timer is most certainly lost and we want to update the settings immediately after clicking the launchgame button (see also the comment in handleGamestartMessage (// Immediately inform the lobby server instead of waiting for the load to finish)

Missing immediate update on player join/leave (Design choice): As rightfully requested by scythetwirler (see bold text above), we should send an update immediately if a client joined, not only if a cooldown period has occured (that might occur after an arbitrary amount of time, potentially unlimited until the game starts if the host is a tard). Lobby players should get an immediate notification if one of their fellows joined a game.

(This isn't really nice to implement because the functions check for g_PlayerAssignments while that wasn't replaced yet with the new assignments when we know whether a client joined or left. Notice that we receive a player-assignment change message each time the ready state is reset, so basically always).

Also 2 seconds seem better than 3 which is really bad latency.

Style remarks:

  • Keep the early return in sendRegisterGameStanzaImmediate the first statement. No need to clear the timer if the current player isn't a controller or if it isn't a lobbied game to begin with (because then the timer doesn't exist).
  • g_DeferredGameStanzaTimer -> The timer implies doing things in a deferred manner, g_GameStanzaTimer is more correct / sound.
  • /** -> /*
  • { on a separate line
  • undefined is more often used than null (would be the first occurance in this file)
  • The JSdoc comment for the global isn't very informative. It should define when this variable is true.
  • The stanza globals should be added near the existing stanza global g_LastGameStanza
  • Don't remove the period from the sendRegisterGameStanzaImmediate comment.

Testing:

  • By using 3 lobby accounts on one unix machine, we can see how often a game becomes updated in the lobby while changing the settings in the other window.

Fixed those, thanks for the patch.

Closing this as fixed and writing a ticket for the serverside implementation, since the server should be immune to this kind of DOSing. (We should still teach our client to not be noisy).

comment:8 by elexis, 7 years ago

Resolution: fixed
Status: newclosed

In 19641:

Vastly reduce the number of gamelist lobby updates sent by the 0 A.D. client (by waiting 2 seconds for the gamesettings to not change before sending an update).
An empty lobby with single client scrolling with the mousewheel through the skirmish maps for few seconds was sufficient to delay the server updates globally for more than 15 seconds.

Based on patch by: sbirmi
Fixes #3847

comment:9 by elexis, 6 years ago

Phab:P39 has a serverside implementation.

Note: See TracTickets for help on using tickets.