-
Notifications
You must be signed in to change notification settings - Fork 179
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
Adding initial commit for the blocking functionality in the get func… #535
base: master
Are you sure you want to change the base?
Conversation
… instead of max time and max attempts. Also cleaned up the function to try removing any that have idles for too long
…t we are checking agains if an object has been idling for too long
I realized that there was a better implementation which I will explain below:
I look forward to hearing any feedback/ responding to questions In response to #519 |
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.
thanks for the PR, a few inline comments and once those are sorted out better inline documentation, unit tests and updating the changelog would be great.
@@ -72,17 +72,27 @@ def get_and_release(self, destroy_on_fail=False) -> Iterator[T]: | |||
raise | |||
self.release(obj) | |||
|
|||
def get(self): | |||
def get(self, **options): |
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.
Why not use keyword arguments with defaults here? That and a docstring will make this feature easier to find and documented in our code docs.
|
||
if self._after_remove is not None: | ||
self._after_remove(obj) | ||
while current_time <= end_time: |
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.
if I understand this correctly this results in busy waiting all while holding a lock. Which seems like a behavior we don't want to introduce.
This will add the blocking functionality to the get function with the following optional parameters:
This is in response to issue #519