RSDK-4455 - Run session heartbeat loop in separate thread #400
Conversation
cheukt
requested review from
maximpertsov,
stuqdog and
njooma
and removed request for
stuqdog
There was a problem hiding this comment.
my test suggestion is [opt] if it ends up being too cumbersome
There was a problem hiding this comment.
This is good! I know @dgottlieb had thoughts on process vs thread
|
after talking to @dgottlieb a bit more, will try to make this a thread |
|
rerequested review since I changed to using threads, which simplifies the code quite a bit |
cheukt
changed the title
cheukt
changed the title
| self._thread = Thread( | ||
| name="heartbeat-thread", | ||
| target=asyncio.run, | ||
| args=(self._heartbeat_process(wait),), | ||
| daemon=True, | ||
| ) | ||
| self._thread.start() |
There was a problem hiding this comment.
Q: is it possible to get here with an active thread? (e.g. self._thread is not None). If so, do we need to do any cleanup?
There was a problem hiding this comment.
I think it is possible, added a reset if so
| self._address = address | ||
| self._dial_options = dial_options | ||
|
|
||
| self._lock: Lock = Lock() | ||
| self._supported: _SupportedState = _SupportedState.UNKNOWN | ||
| self._thread: Optional[Thread] = None |
There was a problem hiding this comment.
It'd be nice to define these above with the other instance vars
There was a problem hiding this comment.
moved, also moved some of the class vars into this function since I don't think they should've been class vars to begin with
| name="heartbeat-thread", | ||
| target=asyncio.run, | ||
| args=(self._heartbeat_process(wait),), | ||
| daemon=True, |
There was a problem hiding this comment.
Are we sure this should be daemonized?
There was a problem hiding this comment.
main issue I ran into a non-daemonized thread is that it doesn't stop on a ctrl+c, and there doesn't seem to be a place to catch to interrupt and kill the thread either? since the daemonic thread just dies on program exit, the cleanup can be handled like normal (since the one rust connection is reused).
src/viam/sessions_client.py
Outdated
|
|
||
| channel = await dial(address=self._address, options=dial_options) | ||
| client = RobotServiceStub(channel.channel) | ||
| while self._supported == _SupportedState.TRUE: |
There was a problem hiding this comment.
Does this check need to be in a lock?
Co-authored-by: Maxim Pertsov <maxim.pertsov@gmail.com>
instead of within the same thread (using the event loop), since that can be crowded out if thread is blocked
only one shared var, and we only need it to track 3 states: True, False, None. Made an enum to track