Refactor message sending interface to be a coroutine #497
Conversation
Askaholic
force-pushed
the
handle-backpressure
branch
from
185fe9e
to
86634d3
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
from
86634d3
to
2abd14b
Compare
| encode_queues(dirty_queues), | ||
| lambda lobby_conn: lobby_conn.authenticated | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This will cause report_dirties to not wait for these broadcasts. Worst case scenario, we'll start doing these in parallel. We need to change report_dirties to a coroutine as well. Also an opportunity to make its infinite loop sane.
There was a problem hiding this comment.
I just took a look at aiocron and this is pretty much exactly how it is implemented. It has a synchronous function which fires off a coroutine without waiting for it to complete, and then schedules its self to be called again via loop.call_at. I tried working on refactoring it to use more or less of a sleep loop, but then you need to have a way of signaling the loop to shut down when the server shuts down, which is really annoying to do. With the call_at method, you don't need a shutdown signal, because if the server shuts down then the coroutine just won't be called again.
Askaholic
force-pushed
the
handle-backpressure
branch
from
2abd14b
to
d4ba9aa
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
from
d4ba9aa
to
125cc79
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
from
fe4e7e8
to
25eae7f
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
2 times, most recently
from
135045b
to
af399d1
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
from
af399d1
to
246661b
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
from
91a8065
to
94ad73f
Compare
Askaholic
force-pushed
the
handle-backpressure
branch
from
6db370a
to
66db33e
Compare
| lambda lobby_conn: lobby_conn.authenticated and validation_func(lobby_conn) | ||
| )) | ||
|
|
||
| await asyncio.gather(*tasks) |
There was a problem hiding this comment.
Is it safe to run all these broadcasts in parallel? Is it possible for data they send to get interleaved?
There was a problem hiding this comment.
Well you have to remember that with asyncio, nothing actually runs in parallel. There is no scheduler that will interrupt a coroutine, so once a coroutine starts, it keeps running until it decides it’s ready to stop. So I don’t imagine that it’s possible for any messages to become corrupted. I would also guess that they’ll likely execute in the order that they’re created, although it wouldn’t be guaranteed.
There was a problem hiding this comment.
Only chunks of code that don't contain any await calls are guaranteed to be atomic, so if we sent any command in chunks using multiple await calls, then they could get interleaved. It does seem that we're only writing whole commands, though.
There was a problem hiding this comment.
Yea, I don't think the code does that anywhere. The QDataStreamProtocol.send_message function always encodes an entire message and writes it with a single call to StreamWriter.write
* Refactor message sending interface to be a coroutine * Use gather to wait on some messages simultaneously * Clean up imports * Call gather correctly * Check results for exceptions * Make abort() a coroutine * Make report_dirties a coroutine so that messages are guaranteed to be sent in order * Add test to verify that ping message is sent * Refactor report dirty interval using aiocron style timer * Refactor a few other unnecessary uses of asyncio.ensure_future * Clean up timer * Revert geoip service change * All commands must be coroutines, added tests for lobbyconnection error conditions * Adjust tests so they actually work * Add test for verifying that your foes don't see your games * Add test for verifying that only your friends can see games with 'friends' visibility state * Make command_pong a coroutine
This refactors the old callback interface for message sending to use async/await native instead. Not only is the coroutine interface cleaner and more intuitive, it also helps to ensure that the code behaves as we expect it to (for instance backpressure handling) without us needing to even think about edge cases.
Also adds a few additional test cases.