-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement exception handling for async generators. #13
Conversation
998a946
to
8ee842d
Compare
8ee842d
to
6c54233
Compare
Fixes DelfinaCare#12. The default behavior is to cache Exceptions. However, there is an option to retry exceptions, which will also respect the concurrency guarentees from once.
6c54233
to
343485d
Compare
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.
Still reviewing but here's what I've got so far
# In the sync case, we also need a sleep(0) for a different reason. While a thread is in the | ||
# WAITING state, it would otherwise hog the Global Interpreter Lock in its while loop until | ||
# the interprer decides to switch threads. With N threads, N - 1 of them would be in the | ||
# WAITING state, and a round-robin interpreter would give each of their while loops the | ||
# thread switching time in execution before reaching the single thread in the GENERATING state. | ||
# Theoretically, this could result in the GIL only working on the thread in the GENERATING | ||
# state 1/Nth of the time, without a time.sleep(0) to manually trigger a GIL switch. In | ||
# practice, removing the time.sleep(0) call would result in large drops in performance when | ||
# running the multithreaded unit tests. |
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.
This description isn't wrong, and if you want to keep it as-is, that's totally fine, but it's basically because we've implemented our own spin-lock and this is a common pattern in a spin-lock for the reasons you mentioned.
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.
I literally couldn't find a single external description at this level of detail about the issues with spinlocks in python, especially when considering the specifics of asyncio or the GIL.
I'd prefer to keep it in here until such time as we can maybe write this up in a blog post and link to it - I really don't want to forget this reasoning.
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.
Sounds good, we can keep it then!
Suggestion from PR comment.
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.
Awesome!
|
||
def __init__(self, *args, **kwargs) -> None: | ||
self.active_generation_completed = asyncio.Event() | ||
super().__init__(*args, **kwargs) |
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.
Is there any reason for interweaving super().init between the two local variables? If there is, a comment might be nice, otherwise I would just call super first.
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.
added a comment.
Fixes #12.