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
reimplemented the MemQueue class to avoid unordered inserts #295
Conversation
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!
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 tried to review, but it's still pretty hard to understand what's going on. Is it possible to add more tests/comments or refactor to make the logic easier to read?
quotient, _ = divmod(mem_queue_duration, queue_duration) | ||
assert quotient < 30 |
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 30? :)
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.
Arbitrary number. It's just set to make sure the class does not get slower over time when we change it
item = "small stuff" | ||
queue = MemQueue( | ||
maxmemsize=get_size(item) * 2 + 1, refresh_interval=0.1, refresh_timeout=30 | ||
) | ||
max_size = 0 |
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 think a comment would help to understand what's going on in the test. I vaguely understand it, but not 100% sure and not 100% sure what is the actual problem.
I can add comments, but hardly refactor because it's based on the |
I see. Maybe pointing out every change to the original implementation can help? |
this is what I did with the two comments (1/ 2/) in the cod, we have two changes -- the original method is here https://github.com/python/cpython/blob/3.10/Lib/asyncio/queues.py#L111 |
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.
LGTM
996d30d
to
8c37a63
Compare
8c37a63
to
3aa23c4
Compare
I closed #281 because I figured out that we did not have a race condition.
See #281 (comment)
But then I realized the implementation can lead to unordered inserts in the queue. So this new patch reintroduces the change which does respect the ordering.