Switch the agent over to UDP #91
Conversation
There was a problem hiding this comment.
Looks good! Thanks again for doing this 😄
My only concern is running the defrag on the reader thread. I think we should aim to have all state access/mutation happen on the agent's main thread so that it's easier to reason about how the state changes and so we can potentially use the fragment state later on for different things. (Let's talk about this in person in the morning!)
| raw_data = io_data.get_data() | ||
| address = io_data.get_address() | ||
|
|
||
| if FragmentUtils.is_fragment(raw_data): |
There was a problem hiding this comment.
I really think this should be running on the main thread, not the reader thread. It'll help avoid subtle concurrency bugs in the future if we're only accessing state on the main thread. Right now only the reader uses the fragment state, but in the future if we ever need to make a decision based on the fragments it'll be easier to have it accessible.
I initially used the handler_function pattern to avoid coupling the executor with these IO classes, but I think it should be fine now especially since it has become a common pattern. One way could be to add an executor argument to the base class and then schedule _received_data() on the executor.
So in InterfaceBase you can add something like:
def _schedule_received_data(self, io_data):
self._executor.submit(self._received_data, io_data)and then in _read_data() instead of calling _received_data(), call self._schedule_received_data(UDPData(...)) (and similarly for STDStreams too). Then the incoming_data_handler can be set to the protocol handle message functions directly.
| def retrieve_io_data(): | ||
| return io_data | ||
|
|
||
| self._incoming_data_handler(retrieve_io_data) |
There was a problem hiding this comment.
Why not just pass io_data directly?
|
Attaching #61 |
| def __init__(self, **kwargs): | ||
| super(Hello, self).__init__( | ||
| InteragentProtocolMessageType.Hello, | ||
| **kwargs |
| def __init__(self, **kwargs): | ||
| super(Bye, self).__init__( | ||
| InteragentProtocolMessageType.Bye, | ||
| **kwargs |
| def __init__(self, **kwargs): | ||
| super(NewOperations, self).__init__( | ||
| InteragentProtocolMessageType.NewOperations, | ||
| **kwargs |
Reraising #90