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

Async::Queue#signal does not default value to nil #341

Closed
oeoeaio opened this issue Aug 26, 2024 · 5 comments
Closed

Async::Queue#signal does not default value to nil #341

oeoeaio opened this issue Aug 26, 2024 · 5 comments
Assignees
Milestone

Comments

@oeoeaio
Copy link

oeoeaio commented Aug 26, 2024

#276 introduced a breaking change to the public interface of Queue. Prior to #276 #signal was inherited from Notification where value is defaulted to nil if it is not provided.

The new implementation of Queue#signal does not provide a default. Our implementation calls this method without an argument, and so now we are getting the following exception:

ArgumentError wrong number of arguments (given 0, expected 1)
@ioquatix
Copy link
Member

Apologies for this breaking change.

Considering your investigation, do you think we should restore the default behaviour? If so, do you mind submitting a PR?

@oeoeaio
Copy link
Author

oeoeaio commented Aug 27, 2024

I can make a PR, but it feels to me like the semantics of the #signal method have fundamentally changed, so I'm not sure I fully understand the expected behaviour. Before, a Queue was a Notification, but now it wraps a Notification (@available). The new #signal implementation is basically an alias for #enqueue, and there is no way to directly signal to the wrapped Notification without the byproduct of adding an item to the queue. If I just default the value to nil then a nil item will be added to the queue, which I don't think it appropriate. It's possible that our implementation is now outdated, and we don't need to call #signal anymore to achieve the same result.

For context our use of #signal is related to #176, which looks to have been resolved by #187. I will test whether the issue still exists in the absence of the #signal call, and if not, I will just remove it from our implementation.

@oeoeaio
Copy link
Author

oeoeaio commented Aug 27, 2024

Confirmed, it looks like v2.3.0 which includes the changes in #187 fixed our issue, and we just haven't checked since then. So we can just remove the #signal call completely. I'm not sure what this means for the breaking change, but hopefully no-one else is using it in the same way we were.

@ioquatix
Copy link
Member

Awesome, let's keep this open and when I have time to review it in detail, I'll make a decision.

@ioquatix
Copy link
Member

I'm okay with following the same contract as Notification#signal.

@ioquatix ioquatix self-assigned this Oct 29, 2024
@ioquatix ioquatix added this to the v2.18.0 milestone Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants