Rework robotd JSON connections #78
Conversation
The 'should_retry' parameter always ends up being 'True', so there's not a lot of point in having it. Iff we find we need it, we can add it back later. For now removing it greatly simplifies reasoning about the system.
This copies over the Connection class from robotd (extending it with types) and uses it to simplify our connection handling. Our version has a notable difference that when it receives an empty message it raises a BrokenPipeError so that this failure is loud. This commit deliberately strips back the retrying support to a minimal single retry which only occurs on errors which are actually fixable by reconnecting. Specifically this means that we now _don't_ catch: - socket.timeout: this would need a very different retry approach than we currently use (we should retry using the same socket, rather than a new one) and isn't likely to be resolvable given that we have a long timeout anyway - OSError: the only error of this class we care about (as far as I'm aware) is FileNotFoundError, which wouldn't be thrown by either the `send` or `receive` methods anyway (only when connecting).
PeterJCLaw
requested review from
Adimote,
RealOrangeOne,
kierdavis and
thomasleese
| pass | ||
|
|
||
| def _connect(self): | ||
| def _get_socket(self): |
There was a problem hiding this comment.
Could we keep the word "connect" in the name of this function? To me, "get socket" sounds like the function returns some already-existing socket rather than creating and connecting a new one.
There was a problem hiding this comment.
I think I was going to have it not connect at one point (just prime the socket), you're right that it's not great as it stands.
I do want it to be clear this is only part of the formation of a connection though. Maybe _get_connect_socket? (feels a bit verbose though).
There was a problem hiding this comment.
I think something like _get_connected_socket would be fine. This is very much a "write-once, read-often" piece of code, so I'd value a descriptive-but-accurate name over a brief one.
| self.data = b'' | ||
|
|
||
| self._connect() | ||
| self.connection = Connection(self._get_socket()) |
There was a problem hiding this comment.
This code is also duplicated in _reconnect. Perhaps it would be neater to initialise connection to None here and then immediately call _reconnect, and modify _reconnect to add a check for connection is not None around the call to connection.close?
There was a problem hiding this comment.
This is deliberate -- I'm precisely moving away from having Nones floating around the place. This is partly because they're annoying and partly because adding types exposes that.
There was a problem hiding this comment.
Makes sense. In that case, it could be factored out into a new method that returns the new Connection?
There was a problem hiding this comment.
e.g.
def _connect(self):
new_connection = Connection(self._get_socket())
self._greeting_response(new_connection.receive())
return new_connection
def __init__(self):
# ...
self.connection = self._connect()
def _reconnect(self):
self.connection.close()
self.connection = self._connect()Assuming self.connection is not modified outside of the above functions, it will never be None.
In theory _greeting_response would need to be modified Assuming the above code is the olAssuming the above code is the olto use new_connection rather than self.connection, though since it's now only used once it could just be inlined into _connect. However I've just seen that our implementation of _greeting_response is simply a no-op, so I think it'd be clearer to just delete it (since clearly robotd does not require nor accept a response to the greeting packet it sends).
There was a problem hiding this comment.
_greeting_response is, I think, provided so that Board implementations can handle the initial status broadcast from robotd. You're right that it's unused at the moment.
I'll have a look at the rest later.
| if message == b'': | ||
| raise BrokenPipeError() | ||
|
|
||
| self.data += message |
There was a problem hiding this comment.
Would it be worth using an io.BufferedReader here instead of implementing buffering ourselves? I haven't used this particular class before, but my intuition suggests we can just call readline on an instance of BufferedReader and it will deal with performing read operations on the socket only when needed.
However, your current approach has the advantage of being more explicit about its error handling. It's not clear without looking at its source how BufferedReader would handle an empty response from socket.recv.
There was a problem hiding this comment.
Interesting, I'll give that a look. I also found that we can turn a socket into a file-like object, though again I'm not sure how that handles errors.
As it stands, this class is pretty much copied from robotd/master.py. If we do find that there's a good wrapper then we should consider using it there, too.
There was a problem hiding this comment.
Tangentially related (and definitely out of scope), but a long-term improvement could be to factor out this logic from both robotd and robot-api into a library used by both (e.g. "robot-protocol"). This would facilitate writing other applications that interact with robotd, such as a manual control interface or the fabled Power Board Orchestra.
There was a problem hiding this comment.
Yup, a common library was another thing I'd wondered about here. I think we'd want to pin down some even higher level things before/while doing that (such as a generic way to pass errors back & forth) too.
I'd hope that something like a manual control interface would sit above the robot-api though -- is there a reason you think it needs to be a sibling?
There was a problem hiding this comment.
I'd hope that something like a manual control interface would sit above the robot-api though
My bad - that would make much more sense that using the hypothetical robot-protocol package directly.
Come to think of it I'm not sure what applications, if any, would use robot-protocol rather than robot-api in this sense.
| Message = Dict[str, Any] | ||
|
|
||
|
|
||
| class Connection: |
There was a problem hiding this comment.
It feels to me like "codec" would be a much better name for this class and module than "connection", especially since this class doesn't actually do any connecting (that's done in Board._get_socket).
There was a problem hiding this comment.
Possibly, though it also does buffering which I don't think a codec would do. As it stands the name is merely the same as the class this is copied from in robotd/master.py. I think it would be good to keep the two classes having the same name, but I don't feel strongly about what the name actually is.
There was a problem hiding this comment.
I agree that it would be good for the two classes to have the same name; as a result, I think we should rename both of them. Let's leave that to another PR, though.
kierdavis
left a comment
kierdavis
left a comment
There was a problem hiding this comment.
This looks like it's functionally correct and a good approach in general to me; however I've left some comments on the style and possible alternatives.
This feels like a change that could really do with a test on real hardware, so I'm not going to "approve" this just yet.
|
@kierdavis thanks! This is definitely in need of some actual testing, as well as a better understanding of some of the errors it's now letting bubble straight away. I'll update the description to clarify my goals as I think there's an even better way of managing the connections, that we've just not yet found. |
This is an attempt to both simplify the logic around the JSON sockets connecting robot-api to robotd and to prepare their usage for having type hints added.
The main unknown I'm facing is not really understanding what real-world scenarios each of the exceptions we currently catch relate to, and thus whether there is anything sensible that we can do in each case.
This branch begins to assume that if we reconnect a socket that we should recommence our instruction cycle with the backend, though doesn't apply this consistently. It's not clear to me how reliable this expectation is; in particular how it affect reconnections which occur in the middle of
_send_and_receive.Goals
Boardin preparation for types, in particular removing or isolating as much lower-level stuff as possibleOptionalby removingNoneassignments to attributesBoardclass and its connection (this PR won't cover all of this, but I mention this as a related goal)