[serve] Call _is_router_running_in_separate_loop() in shutdown_async - #64688
[serve] Call _is_router_running_in_separate_loop() in shutdown_async#64688HrushiYadav wants to merge 2 commits into
Conversation
_is_router_running_in_separate_loop is a method, but shutdown_async referenced it without calling it, so the check was always truthy (a bound method object). As a result shutdown_async always took the asyncio.wrap_future() branch. When the router runs in the same event loop the future is an asyncio.Future, and wrap_future rejects it with a TypeError. The synchronous shutdown() a few lines above already calls the method correctly. Add unit tests covering both the same-loop and separate-loop paths. Signed-off-by: Hrushikesh Yadav <yadavhrushikesh65@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in DeploymentHandle.shutdown_async where _is_router_running_in_separate_loop was evaluated as a property instead of being called as a method, which caused it to always evaluate to True. Unit tests have been added to cover both same-loop and separate-loop shutdown scenarios. The feedback suggests using the real InitHandleOptions dataclass instead of a mock in the test helper to improve robustness and type safety.
| handle = DeploymentHandle.__new__(DeploymentHandle) | ||
| handle._router = Mock() | ||
| handle._router.shutdown = Mock(return_value=shutdown_future) | ||
| handle.init_options = Mock(_run_router_in_separate_loop=run_in_separate_loop) |
There was a problem hiding this comment.
Since InitHandleOptions is a simple dataclass and is already imported in this file, we can use the real class instead of a Mock. This is more robust, type-safe, and avoids unnecessary mocking of data structures.
| handle.init_options = Mock(_run_router_in_separate_loop=run_in_separate_loop) | |
| handle.init_options = InitHandleOptions(_run_router_in_separate_loop=run_in_separate_loop) |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
Keeping this active. Small fix: |
|
ping me when the build passes |
|
@abrarsheikh the premerge failure looks like an infra issue in the build fleet, not this change. This PR only touches two python files (serve/handle.py and a unit test), so it cannot affect the wanda build steps that failed here, which are all C++ core, Java, wheel, and jar compiles. They failed on both the initial run and the retry, and no serve or python test actually ran since they were all still waiting on build artifacts. buildkite/microcheck and buildkite/release both passed on the same commit. Could you re-trigger premerge when you get a chance? |
Why are these changes needed?
DeploymentHandle.shutdown_asyncchecks_is_router_running_in_separate_loopwithout calling it:_is_router_running_in_separate_loopis a method (defined on_DeploymentHandleBase), so referencing it without()evaluates a bound-method object, which is always truthy.shutdown_asynctherefore always takes theasyncio.wrap_future(...)branch.When the router runs in the same event loop,
self._router.shutdown()returns anasyncio.Future, andasyncio.wrap_future()only accepts aconcurrent.futures.Future— so it raisesTypeErrorinstead of awaiting the future.The synchronous
shutdown()a few lines above already calls the method correctly (self._is_router_running_in_separate_loop()), as do the other call sites in the file. This just bringsshutdown_asyncin line.The fix is a one-character change (add
()), plus unit tests covering both the same-loop and separate-loop paths.Related issue number
N/A — found by inspection.
Checks
ruffon the changed files.