client: Handle server disconnects #11
Conversation
charlesbaynham
changed the title
|
@sbourdeauducq would appreciate your thoughts. |
charlesbaynham
force-pushed
the
handle_server_disconnects
branch
from
451d09a
to
56f508a
Compare
Yes, with the background thread - preferably as soon as the connection fails, not when something is called. Maybe the thread should not terminate and keep monitoring the connection somehow, if that's possible to do at all in Python. |
|
Could keep checking the |
|
Actually a better way would be to use the |
|
The main issue will be with the concurrent access from the thread. |
|
OK, I've reimplemented I've exposed the status of all the three clients through I've added tests for the new features. Todo:
|
|
If someone is feeling motivated it might be nice to give If you agree, I'll squash and write it up in the docs. |
sipyco/pc_rpc.py
Outdated
| def get_local_host(self): | ||
| raise NotImplementedError | ||
| with self.__client_lock: | ||
| client_method = getattr(self.__client, name) | ||
| try: | ||
| return client_method(*args, **kwargs) | ||
| except ConnectionError: | ||
| self.__mark_invalid() | ||
| return None | ||
| return tryer |
There was a problem hiding this comment.
There's a bug here: if the Server returns a ConnectionError as the valid, unpacked output of its method this will be misinterpreted as a failed connection. I've added a failing test to highlight this, will have a think as to how to solve.
There was a problem hiding this comment.
This is resolved in the latest commit: Client now raises RPCConnectionError which is a subclass of ConnectionError if an RPC connection error occurs, which allows you to tell the difference between errors arising from the RPC code and errors that were unpacked. This is backwards compatible because it's still a ConnectionError so any code which looks for these will still receive them.
* Handle closed connections from the server. * Refactor BestEffortClient to reuse logic from Client via composition * Add a thread to BestEffortClient which regularly checks for connectivity and attempts to reestablish it if it is lost
charlesbaynham
force-pushed
the
handle_server_disconnects
branch
from
17ce284
to
ca911b0
Compare
|
I notice that there are no release notes in this package. Could add some? |
|
@sbourdeauducq I noticed this one is still waiting. Do you think it's ready for merge? |
|
Sorry, I don't currently have time to properly review this. This is a rather large and complex change and I'm too busy with the Zynq port. |
No worries, just making sure it wasn't forgotten |
The second half of m-labs/artiq#1361, aiming to handle server disconnects sensibly in the rpc clients.
We previously discussed using similar logic to
sync_structto handle disconnects.sync_structtreats a server disconnect as a normal event and just finishes its task. It also invokes a callback, if supplied.However, I don't think that works here because neither the
Clientnor theAsyncioClienthave an infinite loop running, from which we can exit on disconnect. Both objects are queried by the user code, and so both need to signal to the user that the server has closed the connection and it will never work again. I think this is what exceptions are for.I've therefore raised
ConnectionAbortedErrorwhen the user calls methods that interact with the server but the server has shut down.Remaining questions are what to do with
BestEffortClient? This currently returns None for methods that fail due to connection errors, but a closed socket will never be resolved. Should it attempt to reopen the socket each time it's called?