Page MenuHomeWildfire Games

Unique disconnect reasons

Authored by elexis on Jun 4 2018, 5:20 PM.



Often the screen says that one was discnnected for an unknown reason.
Since the disconnect reason is sent as an int from C++ to JS, a timeout will end up as the state NDR_UNKNOWN = 0.
But that disconnect reason is also sent in case of some serverside errors.
It were preferable to debugging developers but also players to distinguish a network timeout from errors.
As a rule we can deduce to keep one separate disconnect reason per call to the disconnect function.

(Notice the disconnect reason is not transmitted reliably, so we can't have reason 0 to point to network timeouts even if we replace all NDR_UNKNOWN disconnects.
See the enet_peer_disconnect_now call in ~CNetClientSession, see
(Was also thinking about moving the strings from JS to C++ right near the enum, maybe in a macro creating the enum and stringmap, as discussed in D1517. But meh, JS is nicer to modify, especially for mods.)

Test Plan

The alternative to not keep introducing new verbose translated error strings is to have one error string and use that everywhere. It's translation work, but not that much.
If there is an error message, it should preferably say more than "Error: Unknown Error".

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jun 4 2018, 5:20 PM
Vulcan added a subscriber: Vulcan.Jun 4 2018, 5:27 PM

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

Link to build:

  • Correctness verified by:
    • reading the code where the disconnect reason is changed from NDR_UNKNOWN to a new reason.
    • comparing new strings to enum values
    • banned myself
  • Completeness verified by searching for Disconnect( and for NDR_UNKNOWN.

Started a game and joined with a second client, but changing the code to call Disconnect with a different enum seems a bit pointless.

While re-viewing my diff I found the following things:

  • The idea of the patch can be protected from being broken by future authors by adding a LOGWARNING in the two Disconnect functions that complains if someone disconnected the other side without informing them of the reason.
  • The Disconnect function of the client is never called.
  • #4443 describes the missing feature of the client informing the server of the disconnect reason.
  • The Disconnect reasons only relate to the server side. The client disconnect reasons will need a separate enum, so the enum should also be relocated, and NetSession should be split into two files.
  • Extended the comment to state why NDR_UNKNOWN should never be sent. It's about prinicple that the server should inform the client when he can, even when those three cases should basically never occur (since they'd be errors).
  • JSI_Network::DisconnectNetworkGame disconnects the client by deleting it.
  • Found DisconnectNow, just in time!
  • The Disconnect function could (should) use the enum type directly and convert to what enet wants. We're not using enet_uint32 anywhere, but that doesnt mean it would be more accurate to use u32 instead of enet_uint32; making the conversion explicit instead of implicit with intermediary.
  • Changing to 2019 before I have to change it to 2020.
elexis updated this revision to Diff 9964.Sep 26 2019, 1:20 PM

See above.

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

Link to build:

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

Linter detected issues:
Executing section Source...

|  56| class·INetSession
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classINetSession{' is invalid C code. Use --std or --language to configure the language.

|  79| class·CNetHost
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCNetHost{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build:

This revision was not accepted when it landed; it landed in state Needs Review.Sep 26 2019, 1:36 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 26 2019, 1:36 PM