Implement UDP communication services for discv5 #816
Conversation
There was a problem hiding this comment.
Architecturally I'm inclined to suggest modeling this in a similar manner to the p2p.transport.Transport abstraction. This allows you to fully abstract away the underlying networking in testing scenarios.
Now, the current model does have this property in that it appears you could just wire up some trio channels and pretend they had networking behind them.
I'd be completely fine with you iterating on this a bit over some pull requests so this doesn't need to block this PR but something worth considering as you start getting deeper into testing this and want to do things like have multiple peers sending messages to each other.
p2p/discv5/channel_services.py
Outdated
| logger = logging.getLogger('p2p.discv5.channel_services.DatagramReceiver') | ||
|
|
||
| async with incoming_datagram_send_channel: | ||
| while True: |
There was a problem hiding this comment.
I think this should be something like while manager.is_running so the loop can exit naturally in the event of a cancellation being triggered externally.
There was a problem hiding this comment.
Will change because it looks cleaner than a while True. But I don't think it actually changes anything as cancellation would just raise an exception during one of the awaits.
| received_datagram = await receive_channel.receive() | ||
|
|
||
| assert received_datagram.datagram == data | ||
| assert received_datagram.sender == sender_address |
There was a problem hiding this comment.
I think you should probably assert that you have property access on these by doing assert received_datagram.sender.address == ... because I think that this will evaluate truthy if sender is just a plain tuple (but I'm not 100% sure)
Yes, the intention is that there are separate services for each processing step (socket, decoding, decryption, handshake, ...) that just receive data from some channel and send it to another. This should make it possible to test all components fully independently. |
jannikluhn
force-pushed
the
udp-channel-services
branch
from
904084b to
db5769a
Compare
Cool, sounds like you've got a vision for this so I'll wait to see how it comes together. |
jannikluhn
force-pushed
the
udp-channel-services
branch
from
2ed5ea9 to
377d7ad
Compare
| @@ -170,7 +170,7 @@ def cancel(self) -> None: | |||
| ... | |||
|
|
|||
|
|
|||
| LogicFnType = Callable[[ManagerAPI, VarArg(), KwArg()], Awaitable[Any]] | |||
| LogicFnType = Callable[..., Awaitable[Any]] | |||
There was a problem hiding this comment.
Couldn't get the type hints to work otherwise. In fact, I think it's impossible to do this properly right now. See python/mypy#5876 (comment)
There was a problem hiding this comment.
I kind of suspected this was going to be the case.
jannikluhn
force-pushed
the
udp-channel-services
branch
from
5a824dc to
88a3127
Compare
| await incoming_datagram_send_channel.send(incoming_datagram) | ||
|
|
||
|
|
||
| @as_service |
There was a problem hiding this comment.
not for this PR but thinking about API. Maybe we could have this decorator optionally accept some arguments.
@as_service(manager=True)
async def UsesManager(manager: ManagerAPI):
...
@as_service
async def DoesNotUseManager():
...Might be nice to not always have to include the manager argument for services that don't use it.
There was a problem hiding this comment.
👍 sounds like a good idea. Most services will also do some logging, so the same could be done for a logger instance for convenience (might be confusing though, not sure).
jannikluhn
force-pushed
the
udp-channel-services
branch
from
88a3127 to
8954a11
Compare
Add two simple services that read and write data to and from a UDP socket.
To-Do
trio_modeenabledCute Animal Picture