-
Notifications
You must be signed in to change notification settings - Fork 687
refactor: split session startup from streaming to enforce add-cancel-end ordering #4545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,9 @@ def __init__(self, session_id: int, session_mgr: SessionManager, **kwargs): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Set by api_server to AsyncEngine.epoch when a request binds a session; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # generate() drops work if stop_all_session() bumped epoch after bind. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.epoch: int | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Serialize per-session lifecycle operations (generate startup, abort, close) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # without affecting cross-session concurrency. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._lifecycle_lock = asyncio.Lock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # event to wait for the session to be active | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._active: asyncio.Event | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._handle = None # inference instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -105,23 +108,31 @@ async def request_handle(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def async_abort(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Abort the session.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.debug(f'[session] Aborting session {self.session_id}, epoch={self.epoch}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._handle is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await self._handle.async_cancel(self.session_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async with self._lifecycle_lock: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.debug(f'[session] Aborting session {self.session_id}, epoch={self.epoch}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Session already closed/reset; treat as a benign no-op. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._session_mgr is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._handle is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await self._handle.async_cancel(self.session_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def async_close(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """End the session.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f'[session] Ending session {self.session_id}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._handle is None and self.step == 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._handle is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await self._active.wait() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async with self.request_handle() as handle: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await handle.async_end(self.session_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except (Exception, asyncio.CancelledError, GeneratorExit) as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.exception(f'[async_close] exception caught: {e}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.reset() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async with self._lifecycle_lock: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f'[session] Ending session {self.session_id}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Already closed/reset; keep end idempotent. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._session_mgr is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._handle is None and self.step == 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._handle is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await self._active.wait() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async with self.request_handle() as handle: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await handle.async_end(self.session_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except (Exception, asyncio.CancelledError, GeneratorExit) as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.exception(f'[async_close] exception caught: {e}') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.reset() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+121
to
+135
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async with self._lifecycle_lock: | |
| logger.info(f'[session] Ending session {self.session_id}') | |
| # Already closed/reset; keep end idempotent. | |
| if self._session_mgr is None: | |
| return | |
| if self._handle is None and self.step == 0: | |
| return | |
| if self._handle is not None: | |
| await self._active.wait() | |
| async with self.request_handle() as handle: | |
| try: | |
| await handle.async_end(self.session_id) | |
| except (Exception, asyncio.CancelledError, GeneratorExit) as e: | |
| logger.exception(f'[async_close] exception caught: {e}') | |
| self.reset() | |
| while True: | |
| active = None | |
| async with self._lifecycle_lock: | |
| logger.info(f'[session] Ending session {self.session_id}') | |
| # Already closed/reset; keep end idempotent. | |
| if self._session_mgr is None: | |
| return | |
| if self._handle is None and self.step == 0: | |
| return | |
| if self._handle is not None: | |
| active = self._active | |
| else: | |
| async with self.request_handle() as handle: | |
| try: | |
| await handle.async_end(self.session_id) | |
| except (Exception, asyncio.CancelledError, GeneratorExit) as e: | |
| logger.exception(f'[async_close] exception caught: {e}') | |
| self.reset() | |
| return | |
| await active.wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential deadlock due to lock ordering:
generate()acquiressession.request_handle()(which setssession._handle/_active) before takingsession._lifecycle_lock. If another task callssession.async_close()between those steps, it can take_lifecycle_lockand then await_active, whilegenerate()is blocked waiting for_lifecycle_lock, leaving_activeunset forever. To avoid this, take_lifecycle_lockbefore enteringrequest_handle(), or ensureasync_close()never waits on_activewhile holding_lifecycle_lock.