search

socketry / async

An awesome asynchronous event-driven reactor for Ruby.
MIT License
2.09k stars 86 forks source link

Use a linked list for the barrier implementation. #192

Closed ioquatix closed 1 year ago

ioquatix commented 1 year ago

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

Contribution

trevorturk commented 1 year ago

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 commented 1 year ago

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

Did you call barrier.wait in a loop?

trevorturk commented 1 year ago

(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!