Page MenuHomeWildfire Games

Cleanup of the FSM and NetMessage code in network.
Needs RevisionPublic

Authored by echotangoecho on May 13 2017, 10:22 PM.

Details

Reviewers
elexis
leper
Summary

The current code for the FSM and the handling (serialization, deserialization) of NetMessages is dated and not very nice. This patch tries to improve both.

Test Plan

Verify that multiplayer games still work well and tests run correctly.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
netmessages
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1707
Build 2700: Vulcan BuildJenkins
Build 2699: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
leper added inline comments.May 15 2017, 2:02 AM
source/network/NetClient.cpp
405

I suspect that we might want to enable writing that on one line again.

865

switch

source/network/NetFileTransfer.cpp
29

switch

source/network/NetFileTransfer.h
21

Just forward decl it instead.

source/network/NetMessageBuffer.cpp
63

These three could just be in the header.

103

Why the first param?

source/network/NetMessageBuffer.h
56

Where do we need the non-const one?

57

That comment seems pointless.

187

At least follow the example in the simulation serialization code and fail explicitly in case anyone ever tries to build on something big endian.

231

Same here.

source/network/NetServer.cpp
1027

switch?

source/network/tests/test_NetMessage.h
110

This seems to slightly miss the point of testing msg2 against a known good string.

source/ps/CStr.cpp
31

is this one still needed?

source/ps/tests/test_CStr.h
109

If you remove these I'd have expecte something similar in the new tests.

echotangoecho added inline comments.May 15 2017, 4:21 PM
source/network/NetMessageBuffer.cpp
103

It is required for the CBinarySerializer, but I don't need it here. Guess I could use UNUSED() here.

source/network/NetMessageBuffer.h
187

I don't think this code depends on endianness? I just serialize the integer to little-endian here, and deserialize it as such.

source/ps/tests/test_CStr.h
109

The serialization / deserialization of strings is already tested in the tests for net messages.

echotangoecho added inline comments.May 18 2017, 7:58 PM
source/network/NetMessageBuffer.h
56

The non-const version is used during deserialization (the members are passed by non-const reference to the deserialization functions).

source/network/tests/test_NetMessage.h
110

The idea here was to only test whether the serialization / deserialization of the messages works correctly, the actual JS serialization tests are (should) already be done in simulation2/tests/test_Serializer.h.

Address some of lepers concerns and rebase.

echotangoecho marked 9 inline comments as done.May 18 2017, 8:06 PM

Build has FAILED

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)...........................................................................................................................................................................................................................................Assertion failure: initialized(), at ../../../libraries/source/spidermonkey/include-unix-debug/js/RootingAPI.h:1161
Segmentation fault

Link to build: http://jw:8080/job/phabricator/1247/
See console output for more information: http://jw:8080/job/phabricator/1247/console

leper requested changes to this revision.May 19 2017, 8:54 PM

A few more comments.

source/network/Fsm.cpp
22

At least some of the codebase has \n\t: m_Foo(bar).

41

I do hope that we do take quite some care (as in fail-safe or rather fail-early-and-loud checks) that the lifetime of that buffer actually is longer than that of the event.

42

Actually this whole implementation is that short (and mostly trivial) that it could just be done in the header, but I guess that is just personal preference.

65

These sound just like header defs, but well.

72

Unless we expect this to handle multiple actions (which would make me question the function name), why not just result = ...? Or even return !m_Action.m_Function || m_Action.m_Function(...)? (The latter might be golfing, but the &= seems pointless.

90

Could use emplace, given that we are really just calling the ctor and passing that to push_back.

115

Guess what ;-)

170

Yeah, well.

source/network/Fsm.h
90

Is anything actually inheriting from this?

source/network/NetMessageBuffer.cpp
103

UNUSED would make make this obvious.

106

const?

source/network/NetMessageBuffer.h
19

You might want to update that for the new filename.

128

Are there many occurences where we need a copy of this?

136

Could be const

187

Ah, right. Sorry for the noise.

387

I do like the comments what #if this closes, especially if there is most of the file in-between.

source/network/NetServer.h
239–253

I might be missing something, but can't event be a reference?

source/network/tests/test_NetMessage.h
36

This,

45

and this seem useful though.

This revision now requires changes to proceed.May 19 2017, 8:54 PM
echotangoecho edited edge metadata.
echotangoecho marked 17 inline comments as done.
  • Fix assert failures in debug mode (add default constructor to SerializeableJSValue).
  • Remove unneeded copy constructor for CSimulationMessage.
  • Fix most (all?) issues found by leper.
source/network/Fsm.cpp
22

It is the questions which we prefer: looking in some of the simulation2 and the network files, there is no break before the :.

41

The buffer is only used from inside the update function where it is used to execute all handlers, and they probably shouldn't depend on the buffer living any longer than for the execution of the function.

source/network/NetHost.cpp
55

The check is moved to the GetData function on the NetMessageBuffer, but it probably still isn't really needed.

source/network/NetMessageBuffer.h
128

only in one place, I have added a data() and size() function for that.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1703/ for more details.

leper requested changes to this revision.Jul 9 2017, 2:59 AM
CFooBarMessage message;
event.GetMessageBuffer().GetMessage(message);

seems like a quite common piece of code that could possibly be handled within the constructor. (It might not be nicer, but I'd like to know why in that case.)

Somewhat related to this patch but I do have another const diff for ScriptInterface lying around, which I guess will only mean replacing ScriptInterface& by const ScriptInterface& depending on what is merged first.

There are still 2 comments (one marked TODO, the other one being about what scriptinterface to use IIRC) that should probably be fixed.

source/network/Fsm.cpp
22

I somewhat dislike having that thing trailing at the end of a long line like that, but if you prefer this, fine by me.

65

That was what that one emplace_back comment below referred to IIRC.

source/network/NetClient.cpp
72–75

nullptr?

77–79

I suspect we should rename that member variable to be consistent with all other uses of GetContext.

source/network/NetFileTransfer.cpp
29

\n{

source/network/NetMessageBuffer.cpp
106

Could this be const std::vector<u8>&? (Would also make the few users of this use the same type, avoiding the need to copy.)

This revision now requires changes to proceed.Jul 9 2017, 2:59 AM
echotangoecho edited edge metadata.
echotangoecho marked 4 inline comments as done.
  • Replace some NULL occurrences with nullptr.
  • push_back -> emplace_back in some places.
  • Fix some more style issues (including some whitespace things).
  • Remove leftover TODO and comment about scriptInterface.
  • Add an easier syntax for deseralizing a message, as suggested by leper.

Fix 4 more whitespace issues.

echotangoecho added inline comments.Jul 30 2017, 12:27 AM
source/network/NetClient.cpp
77–79

Might be something for a later patch?

Fix 2 more whitespace issues, which should hopefully have been the last remaining.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1769/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1770/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1771/ for more details.

leper requested changes to this revision.Jul 30 2017, 9:48 PM

This is looking good so far, I guess this should get quite extensive testing on as many platforms/compilers as we can get.

source/network/Fsm.h
46

Why not NetMessageType?

111

Why aren't we using NetMessageType for eventType? (Applies to the whole class + users; mostly this would remove all the (uint) casts for AddTransition callers.)

source/network/NetClient.cpp
77–79

Fine by me.

837

No need for the cast.

source/network/NetMessageBuffer.cpp
91

What if this overflows?

Yes, this is unlikely to occur, but this is in network code.

137

See the comment marked as done about also having users of GetBuffer use const std::vector<u8>&.

source/network/NetMessages.h
438

Possibly const&, if D739 gets merged first.

source/network/NetServer.cpp
1079

Pointless cast (just check the other users of GetType or NMT_* too).

source/network/NetServerTurnManager.cpp
129

Could nuke the {}.

source/network/NetSession.cpp
142–143

Why does this hunk differ in the (if (msg)) check from the one in net server?

This revision now requires changes to proceed.Jul 30 2017, 9:48 PM