Page MenuHomeWildfire Games
Differential D1513 Diff 6687 source/network/NetSession.cpp

Changeset View

Standalone View

View Options

source/network/NetSession.cpp

/* Copyright (C) 2017 Wildfire Games. /* Copyright (C) 2018 Wildfire Games.
* This file is part of 0 A.D. * This file is part of 0 A.D.
* *
* 0 A.D. is free software: you can redistribute it and/or modify * 0 A.D. is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or * the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version. * (at your option) any later version.
* *
* 0 A.D. is distributed in the hope that it will be useful, * 0 A.D. is distributed in the hope that it will be useful,
Show All 9 Lines
#include "NetSession.h" #include "NetSession.h"
#include "NetClient.h" #include "NetClient.h"
#include "NetServer.h" #include "NetServer.h"
#include "NetMessage.h" #include "NetMessage.h"
#include "NetStats.h" #include "NetStats.h"
#include "lib/external_libraries/enet.h" #include "lib/external_libraries/enet.h"
#include "ps/CLogger.h" #include "ps/CLogger.h"
#include "ps/Profile.h" #include "ps/Profile.h"
#include "ps/ConfigDB.h"
#include "scriptinterface/ScriptInterface.h" #include "scriptinterface/ScriptInterface.h"
const u32 NETWORK_WARNING_TIMEOUT = 2000; const u32 NETWORK_WARNING_TIMEOUT = 2000;
vladislavbelovUnsubmitted
Not Done
Inline Actions

Wouldn't it be good to add a comment, that all timeouts are in milliseconds?

vladislavbelov: Wouldn't it be good to add a comment, that all timeouts are in milliseconds?
const u32 MAXIMUM_HOST_TIMEOUT = std::numeric_limits<u32>::max(); const u32 MAXIMUM_HOST_TIMEOUT = std::numeric_limits<u32>::max();
elexisUnsubmitted
Not Done
Inline Actions

Since this is a magic number that can be too long for clients with good connection (LAN) and good computers and it can be too short for clients with a good connection and a bad computer.

Hence the diff must be considered a workaround and it should actually be either reverted after all causes for drops on loadgame are removed - or reduced to a much smaller number.

elexis: Since this is a magic number that can be too long for clients with good connection (LAN) and…
causativeAuthorUnsubmitted
Not Done
Inline Actions

Agree that this is a temporary fix and threading is a better fix.

causative: Agree that this is a temporary fix and threading is a better fix.
elexisUnsubmitted
Done
Inline Actions

A reader might assume that LONG_TIMEOUT serves the same purpose as NETWORK_WARNING_TIMEOUT just in longer.
This should either have a very short comment stating its purpose, or better be renamed to something like NETWORK_GAMESTART_TIMEOUT.

elexis: A reader might assume that LONG_TIMEOUT serves the same purpose as NETWORK_WARNING_TIMEOUT just…
vladislavbelovUnsubmitted
Not Done
Inline Actions

Do we need empty line between constants? They're all timeout constants.

vladislavbelov: Do we need empty line between constants? They're all timeout constants.
u32 g_NetworkGamestartTimeout = 80000;
elexisUnsubmitted
Not Done
Inline Actions

Now unneeded global.

elexis: Now unneeded global.
static const int CHANNEL_COUNT = 1; static const int CHANNEL_COUNT = 1;
void LongTimeout(ENetPeer* peer, bool longTimeout)
elexisUnsubmitted
Not Done
Inline Actions

Were a macro function nicer? (So that we only have two C++ functions with that semantics instead of three)

elexis: Were a macro function nicer? (So that we only have two C++ functions with that semantics…
causativeAuthorUnsubmitted
Not Done
Inline Actions

Wouldn't a macro function be exactly the same? (macro + 2 methods). Also I prefer functions.

causative: Wouldn't a macro function be exactly the same? (macro + 2 methods). Also I prefer functions.
elexisUnsubmitted
Not Done
Inline Actions

Since this function already contains a macro and since this is the only global function, using a macro seems considerable.
But they have the disadvantage that the newlines need to be scaped which is quite ugly, so agree to leave it as a C++ function.

elexis: Since this function already contains a macro and since this is the only global function, using…
{
#if (ENET_VERSION >= ENET_VERSION_CREATE(1, 3, 4))
if (longTimeout)
enet_peer_timeout(peer, 0, g_NetworkGamestartTimeout, g_NetworkGamestartTimeout);
elexisUnsubmitted
Not Done
Inline Actions

Correct arguments.
http://enet.bespin.org/group__peer.html#gac48f35cdd39a89318a7b4fc19920b21b

The second one is the time that is doubled repeatedly until the third argument is reached.
The third one is the limit of the exponential timeout mechanism (the one that makes the players drop),
the fourth one is the maximum timeout limit independent of the exp mechanism.

(Was wondering if the fourth one could be set to 0, so that more than 40 seconds were allowed if the ping is crap. But it may be smaller than 40 if too good ping. Also not important with that magic number 40s being chosen.)

elexis: Correct arguments. http://enet.bespin.org/group__peer.html#gac48f35cdd39a89318a7b4fc19920b21b…
causativeAuthorUnsubmitted
Not Done
Inline Actions

The enet defaults for the second through fourth parameters are: 32 ms, 5 seconds, and 30 seconds.

causative: The enet defaults for the second through fourth parameters are: 32 ms, 5 seconds, and 30…
else
enet_peer_timeout(peer, 0, 0, 0);
ImarokUnsubmitted
Done
Inline Actions

Trailing whitespace? (looks like 2 tabs)

Imarok: Trailing whitespace? (looks like 2 tabs)
#endif
}
CNetClientSession::CNetClientSession(CNetClient& client) : CNetClientSession::CNetClientSession(CNetClient& client) :
m_Client(client), m_FileTransferer(this), m_Host(NULL), m_Server(NULL), m_Stats(NULL) m_Client(client), m_FileTransferer(this), m_Host(NULL), m_Server(NULL), m_Stats(NULL)
{ {
// g_NetworkGamestartTimeout is used by both client and server, but not used by the server
// until a local client has already been created, so it can be loaded from config here.
float timeout;
elexisUnsubmitted
Not Done
Inline Actions

float is a bit ugly. int almost exclusively has at least 32bit, but almost exclusively is not always, so meh.
http://en.cppreference.com/w/cpp/language/types

elexis: float is a bit ugly. `int` almost exclusively has at least 32bit, but almost exclusively is not…
vladislavbelovUnsubmitted
Not Done
Inline Actions

I agree - float isn't good here, especially if it's casted to the u32 anyway.

vladislavbelov: I agree - `float` isn't good here, especially if it's casted to the `u32` anyway.
elexisUnsubmitted
Not Done
Inline Actions

Just that int is the only integer type supported currently by the config system and we want to be able to allow more than 65535 milliseconds and shouldn't rely on probabilities.

elexis: Just that int is the only integer type supported currently by the config system and we want to…
StanUnsubmitted
Not Done
Inline Actions

Isn't int −2,147,483,648 to 2,147,483,647, ?
−32,768 to 32,767 is for short int.

So could at least be unsigned :)

Stan: Isn't int −2,147,483,648 to 2,147,483,647, ? −32,768 to 32,767 is for short int. So could at…
vladislavbelovUnsubmitted
Not Done
Inline Actions

Anyway float isn't for this case, you always can use uint and clamp it. But do not use this hacky workaround.

vladislavbelov: Anyway `float` isn't for this case, you always can use `uint` and clamp it. But do not use this…
elexisUnsubmitted
Not Done
Inline Actions

Always after implementing the uint conversion in CConfigDB code: D1566.
uint can't be used, as mentioned we should be able to pass a value greater than u16, so we can implement the u32 conversion which already seems to be applicable to many other config values.

elexis: Always after implementing the `uint` conversion in CConfigDB code: D1566. `uint` can't be used…
CFG_GET_VAL("network.gamestarttimeout", timeout);
g_NetworkGamestartTimeout = (u32)timeout;
} }
CNetClientSession::~CNetClientSession() CNetClientSession::~CNetClientSession()
{ {
delete m_Stats; delete m_Stats;
if (m_Host && m_Server) if (m_Host && m_Server)
{ {
▲ Show 20 Lines • Show All 149 Lines • ▼ Show 20 Lines
u32 CNetClientSession::GetMeanRTT() const u32 CNetClientSession::GetMeanRTT() const
{ {
if (!m_Server) if (!m_Server)
return 0; return 0;
return m_Server->roundTripTime; return m_Server->roundTripTime;
} }
void CNetClientSession::SetLongTimeout(bool longTimeout)
{
LongTimeout(m_Server, longTimeout);
}
CNetServerSession::CNetServerSession(CNetServerWorker& server, ENetPeer* peer) : CNetServerSession::CNetServerSession(CNetServerWorker& server, ENetPeer* peer) :
m_Server(server), m_FileTransferer(this), m_Peer(peer) m_Server(server), m_FileTransferer(this), m_Peer(peer)
{ {
} }
u32 CNetServerSession::GetIPAddress() const u32 CNetServerSession::GetIPAddress() const
{ {
▲ Show 20 Lines • Show All 43 Lines • ▼ Show 20 Linesvoid CNetServerSession::SetLocalClient(bool isLocalClient)
m_IsLocalClient = isLocalClient; m_IsLocalClient = isLocalClient;
if (!isLocalClient) if (!isLocalClient)
return; return;
// Prevent the local client of the host from timing out too quickly // Prevent the local client of the host from timing out too quickly
#if (ENET_VERSION >= ENET_VERSION_CREATE(1, 3, 4)) #if (ENET_VERSION >= ENET_VERSION_CREATE(1, 3, 4))
enet_peer_timeout(m_Peer, 0, MAXIMUM_HOST_TIMEOUT, MAXIMUM_HOST_TIMEOUT); enet_peer_timeout(m_Peer, 0, MAXIMUM_HOST_TIMEOUT, MAXIMUM_HOST_TIMEOUT);
#endif #endif
elexisUnsubmitted
Not Done
Inline Actions

The function should be called here too

elexis: The function should be called here too
causativeAuthorUnsubmitted
Not Done
Inline Actions

I think that map generation and having a local client are separate cases that need different timeouts.

causative: I think that map generation and having a local client are separate cases that need different…
} }
void CNetServerSession::SetLongTimeout(bool longTimeout)
{
// Don't change the timeout if the client is local, since that is handled by SetLocalClient.
if (m_IsLocalClient)
elexisUnsubmitted
Done
Inline Actions

(IMO better to have either code or comment per line, even if not exceeding the recommended 80 char linelength)

elexis: (IMO better to have either code or comment per line, even if not exceeding the recommended 80…
return;
LongTimeout(m_Peer, longTimeout);
}
elexisUnsubmitted
Done
Inline Actions

Should be possible to have only one copy of that code by passing the enet peer as an argument. Could be either a macro or a void in the scope of the translation unit.

elexis: Should be possible to have only one copy of that code by passing the enet peer as an argument.
Wildfire Games · Phabricator