Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Rework robotd JSON connections #78

Open
PeterJCLaw wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Rework robotd JSON connections #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8bb1b2f
Remove unused attribute
PeterJCLaw Feb 17, 2018
b091052
This timeout applies in both directions
PeterJCLaw Feb 17, 2018
0be9c46
Drop redundant parameter
PeterJCLaw Feb 17, 2018
fd2c6af
Simplify robotd connections
PeterJCLaw Feb 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 32 additions & 86 deletions robot/board.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,23 @@
import json
import logging
import socket
import time
from pathlib import Path
from typing import Mapping, TypeVar, Union

from .connection import Connection, Message

LOGGER = logging.getLogger(__name__)


class Board:
"""Base class for connections to ``robotd`` board sockets."""

SEND_TIMEOUT_SECS = 6
RECV_BUFFER_BYTES = 2048
CONNECTION_TIMEOUT_SECS = 6

def __init__(self, socket_path: Union[Path, str]) -> None:
self.socket_path = Path(socket_path)
self.socket = None
self.data = b''

self._connect()
self.connection = Connection(self._get_socket())
Copy link
Member

@kierdavis kierdavis Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@PeterJCLaw PeterJCLaw Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kierdavis kierdavis Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case, it could be factored out into a new method that returns the new Connection?

Copy link
Member

@kierdavis kierdavis Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@PeterJCLaw PeterJCLaw Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

self._greeting_response(self.connection.receive())

@property
def serial(self):
Expand All @@ -34,23 +32,28 @@ def _greeting_response(self, data):
"""
pass

def _connect(self):
def _get_socket(self):
Copy link
Member

@kierdavis kierdavis Feb 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@PeterJCLaw PeterJCLaw Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@kierdavis kierdavis Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Connect or reconnect to a socket.

:param socket_path: Path for the unix socket
"""
self.socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
self.socket.settimeout(self.SEND_TIMEOUT_SECS)
sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
sock.settimeout(self.CONNECTION_TIMEOUT_SECS)

try:
self.socket.connect(str(self.socket_path))
sock.connect(str(self.socket_path))
except ConnectionRefusedError as e:
LOGGER.exception("Error connecting to: '%s'", self.socket_path)
raise

greeting = self._receive()
self._greeting_response(greeting)
return sock

def _reconnect(self) -> None:
self.connection.close()

self.connection = Connection(self._get_socket())
self._greeting_response(self.connection.receive())

def _get_lc_error(self) -> str:
"""
Expand All @@ -63,97 +66,40 @@ def _get_lc_error(self) -> str:
path=self.socket_path,
)

def _socket_with_single_retry(self, handler):
retryable_errors = (
socket.timeout,
BrokenPipeError,
OSError,
ConnectionResetError,
)

backoffs = [
0.1,
0.5,
1.0,
2.0,
3.0,
]

try:
return handler()
except retryable_errors as e:
original_exception = e

for backoff in backoffs:
time.sleep(backoff)

try:
self._connect()
except (ConnectionRefusedError, FileNotFoundError):
continue

try:
return handler()
except retryable_errors:
pass

raise original_exception

def _send(self, message, should_retry=True):
def _send(self, message: Message) -> None:
"""
Send a message to robotd.

:param retry: used internally
:param message: message to send
"""
try:
self.connection.send(message)
except (BrokenPipeError, ConnectionError):
self._reconnect()
self.connection.send(message)

data = (json.dumps(message) + '\n').encode('utf-8')

def sendall():
self.socket.sendall(data)

if should_retry:
return self._socket_with_single_retry(sendall)
else:
return sendall()

def _recv_from_socket(self, size):
data = self.socket.recv(size)
if data == b'':
raise BrokenPipeError()
return data

def _receive(self, should_retry=True):
def _receive(self):
"""
Receive a message from robotd.
"""
while b'\n' not in self.data:
if should_retry:
message = self._socket_with_single_retry(
lambda: self._recv_from_socket(4096),
)
else:
message = self._recv_from_socket(4096)

self.data += message

line = self.data.split(b'\n', 1)[0]
self.data = self.data[len(line) + 1:]

return json.loads(line.decode('utf-8'))
try:
return self.connection.receive()
except (BrokenPipeError, ConnectionError):
self._reconnect()
return self.connection.receive()

def _send_and_receive(self, message, should_retry=True):
def _send_and_receive(self, message):
"""
Send a message to robotd and wait for a response.
"""
self._send(message, should_retry)
return self._receive(should_retry)
self._send(message)
return self._receive()

def close(self):
"""
Close the the connection to the underlying robotd board.
"""
self.socket.detach()
self.connection.close()

def __str__(self):
return "{} - {}".format(self.__name__, self.serial)
Expand Down
2 changes: 1 addition & 1 deletion robot/camera.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def see(self) -> ResultList:

while True:
try:
return self._see_to_results(self._receive(should_retry=True))
return self._see_to_results(self._receive())
except socket.timeout:
if time.time() > abort_after:
raise
42 changes: 42 additions & 0 deletions robot/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import json
import socket
from typing import Any, Dict

Message = Dict[str, Any]


class Connection:
Copy link
Member

@kierdavis kierdavis Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@PeterJCLaw PeterJCLaw Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kierdavis kierdavis Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
A connection to a device.
This wraps a ``socket.socket`` providing encoding and decoding so that
consumers of this class can send and receive JSON-compatible typed data
rather than needing to worry about lower-level details.
"""

def __init__(self, socket: socket.socket) -> None:
"""Wrap the given socket."""
self.socket = socket
self.data = b''

def close(self) -> None:
"""Close the connection."""
self.socket.close()

def send(self, message: Message) -> None:
"""Send the given JSON-compatible message over the connection."""
line = json.dumps(message).encode('utf-8') + b'\n'
self.socket.sendall(line)

def receive(self) -> Message:
"""Receive a single message from the connection."""
while b'\n' not in self.data:
message = self.socket.recv(4096)
if message == b'':
raise BrokenPipeError()

self.data += message
Copy link
Member

@kierdavis kierdavis Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@PeterJCLaw PeterJCLaw Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kierdavis kierdavis Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@PeterJCLaw PeterJCLaw Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@kierdavis kierdavis Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

line = self.data.split(b'\n', 1)[0]
self.data = self.data[len(line) + 1:]

return json.loads(line.decode('utf-8'))