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

Redefine aliases for LimitedQueue #367

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

jahfer
Copy link
Contributor

@jahfer jahfer commented Jan 3, 2025

👋🏼 I stumbled into this bugfix by only partially reading the documentation 😅

I was playing with Async::LimitedQueue, but called #pop on it (which is technically not documented), and it completely hung. Reason being that the @full notification doesn't fire on #pop because it's referring to the implementation on Async::Queue rather than using Async::LimitedQueue#dequeue, which correctly signals @full.

As a fix, I redefined the aliases in Async::LimitedQueue so they pick up the redefinitions. Without the changes, the updated tests will completely hang in the #pop case.

I also noticed the original test for this wasn't actually working because #contain_exactly wasn't defined, and so the async block containing the assertions was being completely ignored. I couldn't get the structure of what was there prior to work (even with switching to #have_value), so I asserted based on the dequeued values instead. No strong opinion on this :)

Types of Changes

  • Bug fix.

Contribution

@ioquatix
Copy link
Member

ioquatix commented Jan 3, 2025

TBH, aliases and sub-classes are kind of confusing in Ruby leading to behaviour like this. Thanks for the PR, I think it looks okay, but I might replace the aliases with direct dispatch (e.g. def <<(...); self.push(...); end)

@ioquatix ioquatix merged commit e9cc6f0 into socketry:main Jan 3, 2025
25 of 30 checks passed
@ioquatix
Copy link
Member

ioquatix commented Jan 3, 2025

FYI, I'm slowly leaning away from using Async::Queue and so on. During Async v1, they were useful, but in Async v2, I believe it is just better to use Thread::Queue and so on. Althought, while I have not tested it, there may be a small performance advantage to using Async::Queue etc.

@ioquatix
Copy link
Member

ioquatix commented Jan 3, 2025

Okay, I did this: a91f32c

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.

2 participants