Skip to content

Use a linked list for the barrier implementation. #192

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

Merged
merged 2 commits into from
Oct 31, 2022
Merged

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Oct 31, 2022

This simplifies the implementation and unifies the semantics with other concurrency primitives that also use a linked list.

It introduces a minor breaking change, in that Barrier#wait will no longer raise exceptions. Because previously the code would have been to wrap this in exception handling and retry, this should be objectively safer with no downside for existing correct code, and fix hidden issues in code which does not have retries.

Types of Changes

  • Bug fix.
  • New feature.
  • Breaking change.

Contribution

@ioquatix ioquatix force-pushed the barrier-linked-list branch 2 times, most recently from 1507503 to ff67846 Compare October 31, 2022 07:13
@ioquatix ioquatix force-pushed the barrier-linked-list branch from ff67846 to 14229a6 Compare October 31, 2022 07:31
@ioquatix ioquatix merged commit c57a94e into main Oct 31, 2022
@ioquatix ioquatix deleted the barrier-linked-list branch October 31, 2022 07:39
@trevorturk
Copy link
Contributor

trevorturk commented Nov 2, 2022

I noticed this commit and tested with my app, and it's a breaking change for me. I was relying on Barrier#wait to raise exceptions. What's the recommended alternative if I have a barrier and I want exceptions in the barrier's tasks to propagate so that I can rescue and handle them? (Apologies if I was doing this incorrectly before, but I thought this was the way!)

[edit]

I've been playing around with this, and looking at the following (new) docs: https://github.com/socketry/async/tree/main/guides/asynchronous-tasks#exception-handling

I think I'm able to get back to the behavior I've been relying on like so:

barrier.wait # old
barrier.tasks.each { |task| task.task.wait } # new

...but I fear I may be misunderstanding this PR and the motivation for the changes. In my mind, it seems fine for Task#wait to raise exceptions for a single task and Barrier#wait to raise exceptions for the tasks within the barrier? I'm curious if you want to keep this change, if we might add a convenience method like Barrier#wait_all or something along those lines that could be suggested in the exception handling docs?

@ioquatix
Copy link
Member Author

ioquatix commented Nov 2, 2022

Yes, if this breaks code we may need to change it back or add an exception: true/false argument or a separate method. Thanks for your report.

Can you tell me how you were waiting on existing tasks?

I see two issues

  • Barrier#tasks is now longer a direct array of tasks - we can fix this.
  • Barrier#wait no longer raises exceptions.

Did you call barrier.wait in a loop?

@trevorturk
Copy link
Contributor

(If anyone else is reading this, I sent some code samples to Samuel privately to answer his usage questions.)

It looks like #197 restores the preexisting behavior, and #195 introduces Async::Group#wait which also seems interesting for my use cases. I'll keep an eye on these changes and continue to test as things move forward. Thank you!

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