Issue/#771 send broadcast info to rabbitmq

[Askaholic]

I refactored the broadcasting logic to its own service which now also posts the broadcast messages to rabbitmq. I stuck with posting them to the same exchange as the gameResults for now, but I'd be interested to hear other peoples opinions on whether that makes sense or it should get its own exchange. The 3 part routing key is a little weird since there will never be any success states other than "success" and the CRUD operation will always be "update" and there is no real clear way to specify that the message is a broadcast.

I also decided to aggregate all of the game info into a single message just like it is sent during login. Maybe we should take the opportunity to differentiate between "game_info" messages that represent a single game and "game_info" messages that have a "games" sub-list. Maybe call it "game_list_info", or just get rid of the single game version entirely. I think getting rid of the single game version makes a lot of sense as it would maintain consistency with how the other broadcasted info messages look.

Closes #771

[codecov]

Codecov Report

Merging #779 (65207fb) into develop (5dba1f9) will decrease coverage by 0.13%.
The diff coverage is 98.59%.

Impacted Files	Coverage Δ
server/timing/timer.py	79.36% <85.71%> (-6.60%)	⬇️
server/__init__.py	91.02% <100.00%> (-1.98%)	⬇️
server/broadcast_service.py	100.00% <100.00%> (ø)
server/config.py	100.00% <100.00%> (ø)
server/game_service.py	99.02% <100.00%> (+<0.01%)	⬆️
server/lobbyconnection.py	93.96% <100.00%> (ø)
server/message_queue_service.py	97.82% <100.00%> (ø)
server/player_service.py	99.20% <100.00%> (-0.01%)	⬇️
server/timing/__init__.py	100.00% <100.00%> (ø)
... and 3 more

[Brutus5000]

I don't understand much about the structure of the Python server.
So my first question is: Why do you need a "BroadcastService" and does it really do what the name implies?

It looks like the broadcast service is not only responsible for broadcasting but also responsible for fetching the dirty games and also clearing them. For me this looks like a violation of separation of concerns.

If the broadcast service follows it's name it should be responsible for broadcasting messages and the services calling it are responsible for delivering the data. If that's not the case, maybe a different name would be better.

In any scenario the gameService (?) should be responsible for clearing it's own dirtylist.
Whether it pushes the list before cleaning it to the broadcast service or whether it offers a "transactional" method for querying and cleaning the dirty games list to be called by the broadcast service doesn't matter.

[Askaholic]

Why do you need a "BroadcastService"

Mostly this is a refactor of the broadcast loop that used to be part of ServerInstance. It didn't really feel right to keep adding to the code where it was.

whether it offers a "transactional" method for querying and cleaning the dirty games list to be called by the broadcast service

I could do that. That's probably nicer than accessing the member variables directly. There's also this bit of code which IMO doesn't belong there either

if game.state == GameState.ENDED:
    self.game_service.remove_game(game)

but I'd rather not mess with refactoring that ATM since it's out of the scope of this PR and getting it wrong could cause all sorts of horrible stuff like ghost games.

[eoinnoble]

This is way over my head in most respects but certainly nothing glaringly wrong from what I can see.

[eoinnoble]

As someone new to the code base I'd love a brief docstring here.

[Askaholic]

How about that?

[eoinnoble]

Suggested change

	# The reference to the ServerInstance needs to be established later
	service = BroadcastService(
	None,
	message_queue_service,
	game_service,
	player_service,
	)
	service = BroadcastService(
	None, # Reference to the Server instance needs to be established later
	message_queue_service,
	game_service,
	player_service,
	)

[0x647262]

LGTM!