From a8d78a3b26e0f1c2d443f24c755aff0d58ce16b8 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Fri, 8 Mar 2024 09:27:42 -0500 Subject: [PATCH] fix(robot-server): dont let /instruments block (#14608) It calls cache_instruments and that can block because it takes the motion lock, but we really don't want that. We don't mind if cache_instruments doesn't get called on the flex because it's sort of a secondary functionality, so just bail early in this case. ## Testing - Run on a robot and do some `GET /instruments` while things are homign and note that it instantly returns - Do an attach flow and note that it still returns the new instruments. Closes EXEC-298 --- api/src/opentrons/hardware_control/api.py | 4 +++- api/src/opentrons/hardware_control/ot3api.py | 6 +++++- .../hardware_control/protocols/instrument_configurer.py | 1 + robot-server/robot_server/instruments/router.py | 2 +- robot-server/tests/instruments/test_router.py | 8 ++++---- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/api/src/opentrons/hardware_control/api.py b/api/src/opentrons/hardware_control/api.py index 4b62eba7e3a..7267281b247 100644 --- a/api/src/opentrons/hardware_control/api.py +++ b/api/src/opentrons/hardware_control/api.py @@ -431,7 +431,9 @@ def has_gripper(self) -> bool: return False async def cache_instruments( - self, require: Optional[Dict[top_types.Mount, PipetteName]] = None + self, + require: Optional[Dict[top_types.Mount, PipetteName]] = None, + skip_if_would_block: bool = False, ) -> None: """ Scan the attached instruments, take necessary configuration actions, diff --git a/api/src/opentrons/hardware_control/ot3api.py b/api/src/opentrons/hardware_control/ot3api.py index 2190f1b5c4d..ced88815ec9 100644 --- a/api/src/opentrons/hardware_control/ot3api.py +++ b/api/src/opentrons/hardware_control/ot3api.py @@ -653,12 +653,16 @@ def get_all_attached_instr(self) -> Dict[OT3Mount, Optional[InstrumentDict]]: # TODO (spp, 2023-01-31): add unit tests async def cache_instruments( - self, require: Optional[Dict[top_types.Mount, PipetteName]] = None + self, + require: Optional[Dict[top_types.Mount, PipetteName]] = None, + skip_if_would_block: bool = False, ) -> None: """ Scan the attached instruments, take necessary configuration actions, and set up hardware controller internal state if necessary. """ + if skip_if_would_block and self._motion_lock.locked(): + return async with self._motion_lock: skip_configure = await self._cache_instruments(require) if not skip_configure or not self._configured_since_update: diff --git a/api/src/opentrons/hardware_control/protocols/instrument_configurer.py b/api/src/opentrons/hardware_control/protocols/instrument_configurer.py index 810caad667b..ab5b37acc99 100644 --- a/api/src/opentrons/hardware_control/protocols/instrument_configurer.py +++ b/api/src/opentrons/hardware_control/protocols/instrument_configurer.py @@ -28,6 +28,7 @@ def reset_instrument(self, mount: Optional[MountArgType] = None) -> None: async def cache_instruments( self, require: Optional[Dict[Mount, PipetteName]] = None, + skip_if_would_block: bool = False, ) -> None: """ Scan the attached instruments, take necessary configuration actions, diff --git a/robot-server/robot_server/instruments/router.py b/robot-server/robot_server/instruments/router.py index 1497b274a60..561e295a8d1 100644 --- a/robot-server/robot_server/instruments/router.py +++ b/robot-server/robot_server/instruments/router.py @@ -216,7 +216,7 @@ async def _get_attached_instruments_ot3( hardware: OT3HardwareControlAPI, ) -> PydanticResponse[SimpleMultiBody[AttachedItem]]: # OT3 - await hardware.cache_instruments() + await hardware.cache_instruments(skip_if_would_block=True) response_data = await _get_instrument_data(hardware) return await PydanticResponse.create( content=SimpleMultiBody.construct( diff --git a/robot-server/tests/instruments/test_router.py b/robot-server/tests/instruments/test_router.py index b67f24a14cd..8d45c10c5d8 100644 --- a/robot-server/tests/instruments/test_router.py +++ b/robot-server/tests/instruments/test_router.py @@ -121,7 +121,7 @@ async def test_get_all_attached_instruments( subsystem=SubSystem.pipette_right, ) - async def rehearse_instrument_retrievals() -> None: + async def rehearse_instrument_retrievals(skip_if_would_block: bool = False) -> None: decoy.when(ot3_hardware_api.attached_gripper).then_return( cast( GripperDict, @@ -188,9 +188,9 @@ async def rehearse_instrument_retrievals() -> None: # We use this convoluted way of testing to verify the important point that # cache_instruments is called before fetching attached pipette and gripper data. - decoy.when(await ot3_hardware_api.cache_instruments()).then_do( - rehearse_instrument_retrievals - ) + decoy.when( + await ot3_hardware_api.cache_instruments(skip_if_would_block=True) + ).then_do(rehearse_instrument_retrievals) decoy.when(ot3_hardware_api.get_instrument_offset(mount=OT3Mount.LEFT)).then_return( PipetteOffsetSummary( offset=Point(1, 2, 3),