Fix(session): Resolve race condition in automatic restart logic#210
Closed
Itsydv wants to merge 2 commits into
Closed
Fix(session): Resolve race condition in automatic restart logic#210Itsydv wants to merge 2 commits into
Itsydv wants to merge 2 commits into
Conversation
Contributor
|
You should change indent to 4 spaces to maintain consistence between files |
Owner
|
Fixed in 0396151 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, a network error (like TimeoutError or OSError) during an API call in
invoke()would trigger an automatic session restart. However, the implementation of this feature was flawed, leading to a critical race condition.The Issue:
The
invoke()method created therestart()operation as a "fire-and-forget" background task and then immediately retried the failed request recursively. This meant that the session was attempting to send data over a network connection that was simultaneously being closed and reopened by therestart()task.This conflict resulted in a fatal
asyncio.RuntimeError: read() called while another coroutine is already waiting for incoming data, causing the client to go unresponsive.The Fix:
This commit resolves the race condition by serializing the error handling, restart, and retry operations, ensuring they happen in the correct order. The fix consists of three main parts:
asyncio.Lockfor Mutual Exclusion: Anasyncio.Lock(_restart_lock) has been added to theSessionclass. Therestart()method is now wrapped in this lock, which guarantees that only one restart operation can execute at a time, even if multiple network errors occur in quick succession.Synchronous Restart in
invoke(): The exception handler in theinvoke()method has been modified to directlyawait self.restart(). This forces the function to pause and wait for the connection to be fully and cleanly re-established before proceeding.Cleaner Retry Loop: The recursive retry call (
return await self.invoke(...)) has been replaced with acontinuestatement. After a successful restart, this cleanly re-enters thewhile Trueloop to attempt the request again on the new, stable connection.