Skip to content
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

fix: use asyncio.Lock over Event #1095

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: use asyncio.Lock over Event #1095

wants to merge 5 commits into from

Conversation

jackwotherspoon
Copy link
Collaborator

We are currently improperly using asyncio.Event in our code. Event is meant to be used with the wait coroutine to hold coroutine from running until the set() is called. We don't use wait anywhere in our code thus this is not having the intended effect.

Much easier to just use asyncio.Lock which allows usage as a context manager to acquire/release the mutex lock.

@jackwotherspoon jackwotherspoon self-assigned this May 30, 2024
@jackwotherspoon jackwotherspoon requested a review from a team as a code owner May 30, 2024 21:48
@jackwotherspoon jackwotherspoon changed the title fix: use proper asyncio.Lock fix: use proper asyncio.Lock over Event May 30, 2024
@jackwotherspoon jackwotherspoon changed the title fix: use proper asyncio.Lock over Event fix: use asyncio.Lock over Event May 30, 2024
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to carefully audit all the reads and writes to self._current and self._next. I'd expect to see us holding the lock whenever we read or write from those values. WDYT?

@@ -138,7 +138,7 @@ async def force_refresh(self) -> None:
Forces a new refresh attempt immediately to be used for future connection attempts.
"""
# if next refresh is not already in progress, cancel it and schedule new one immediately
if not self._refresh_in_progress.is_set():
if not self._lock.locked():
self._next.cancel()
self._next = self._schedule_refresh(0)
Copy link
Contributor

@hessjcg hessjcg Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Go and Java connectors use the mutex to protect access and modification of self._next and self._current to avoid race conditions that occur when there are concurrent requests to refresh from multiple threads. The python connector probably needs to do the same. See Go RefreshAheadCache instantiation and scheduleRefresh()

@jackwotherspoon
Copy link
Collaborator Author

jackwotherspoon commented Jun 10, 2024

Still have a couple more spots to acquire the lock, the tricky one is in the RefreshAheadCache constructor as Python __init__ functions are sync by default. Might have to add an async class method and use it to initialize the cache so that I can properly use the lock.

self._lock = asyncio.Lock()
self._current: asyncio.Task = self._schedule_refresh(0)
self._next: asyncio.Task = self._current

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants