Skip to content

Add Async::Waiter #174

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

Closed
wants to merge 1 commit into from
Closed

Add Async::Waiter #174

wants to merge 1 commit into from

Conversation

zhulik
Copy link

@zhulik zhulik commented Jul 30, 2022

WIP

As discussed here I introduce a new class called Waiter which can be a drop-in replacement Barrier. The only difference is that it can await for first N tasks.

Once and if the approach is approved I'll add more specific tests

Questions:

  • Better name?
  • Replace Barrier?
  • How to get rid of warnings in logs when a task raises an exception? See example:
Async do |task|
  waiter = Async::Waiter.new
  waiter.async do
    task.sleep(1)
    raise StandardError, "Error!"
  end
  waiter.async do
    task.sleep(2)
    2
  end
  waiter.async do
    task.sleep(3)
    3
  end
  waiter.async do
    task.sleep(3)
    raise StandardError, "Error!"
  end

  done, pending = waiter.wait_for(2)

  done.each do |task|
    Console.logger.info(self, "Done result: #{task.wait}")
  rescue StandardError => e
    Console.logger.info(self, e)
  end

  pending.each do |task|
    Console.logger.info(self, "Pending result: #{task.wait}")
  rescue StandardError => e
    Console.logger.info(self, e)
  end
  Console.logger.info(self, "Done")
end

Even though the tasks with raises have eventually been awaited, I still see warnings in the logs:

  0.0s     warn: Async::Task [oid=0x8c] [ec=0xa0] [pid=453505] [2022-07-30 23:31:16 +0200]
               | Task may have ended with unhandled exception.
               |   StandardError: Error!
               |   → ./waiter.rb:56 in `block (2 levels) in <main>'
               |     ./waiter.rb:20 in `block in async'
               |     /home/zhulik/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/async-2.0.3/lib/async/task.rb:255 in `block in schedule'
  1.0s     info: Object [oid=0xb4] [ec=0xc8] [pid=453505] [2022-07-30 23:31:17 +0200]
               |   StandardError: Error!
               |   → ./waiter.rb:56 in `block (2 levels) in <main>'
               |     ./waiter.rb:20 in `block in async'
               |     /home/zhulik/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/async-2.0.3/lib/async/task.rb:255 in `block in schedule'
  1.0s     info: Object [oid=0xb4] [ec=0xc8] [pid=453505] [2022-07-30 23:31:17 +0200]
               | Done result: 2
  2.0s     info: Object [oid=0xb4] [ec=0xc8] [pid=453505] [2022-07-30 23:31:18 +0200]
               | Pending result: 3
  2.0s     info: Object [oid=0xb4] [ec=0xc8] [pid=453505] [2022-07-30 23:31:18 +0200]
               |   StandardError: Error!
               |   → ./waiter.rb:68 in `block (2 levels) in <main>'
               |     ./waiter.rb:20 in `block in async'
               |     /home/zhulik/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/async-2.0.3/lib/async/task.rb:255 in `block in schedule'
  2.0s     info: Object [oid=0xb4] [ec=0xc8] [pid=453505] [2022-07-30 23:31:18 +0200]
               | Done

Note: I'm leaving on vacation on August 2, the day after tomorrow and won't be able to code till August 9.

Types of Changes

  • New feature.

Testing

  • I added tests for my changes.
  • I tested my changes locally.
  • I tested my changes in staging.
  • I tested my changes in production.

@@ -1,17 +1,17 @@
# frozen_string_literal: true

# Copyright, 2019, by Samuel G. D. Williams. <http://www.codeotaku.com>
#
#
Copy link
Author

@zhulik zhulik Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you say if I start slowly introducing Rubocop?
I've already tried, got about 7.5k(!) issues, .rubocop_todo.yml was about 700 lines long.

I could start with simple cases likes this, missing newlines at the end of files or enforcing tabs for indentation(no need to fix, code is already properly formatted afair) branch

In all my projects I use Rubocop with almost default config (with rubocop-rake, rubocop-rspec, rubocop-performance where applicable) in combination with overcommit to prevent bad code from being committed.

I know many cops are controversial, so it will be up to you whether to use them or not. But I believe autoformatting and some other basic things is a must have. To me working without rubocop is like working with only one hand and only one half of the brain

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, I misread that as 75k issues lol.

When I tried RuboCop a while ago, IIRC it did not support tabs for indentation.

However, I believe that changed recently.

I'd be okay with introducing RuboCop but it would probably need to be the opposite way for me personally, it would need to disable every rule by default and we selectively enable ones that make sense. It would probably just be a small subset and if I did go in that direction I'd want to replicate it to other projects too.


require 'async/clock'
require 'async/rspec'
require 'async/semaphore'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about zeitwerk?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autoload is broken on Ruby with the fiber scheduler so it's a no go for now, but maybe later?

wait_for(n).first.map(&:wait)
end
end
end
Copy link
Author

@zhulik zhulik Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about Rubocop

@ioquatix
Copy link
Member

ioquatix commented Jul 30, 2022

This looks good to me - I like using a sub-class since you need to opt into it - there is extra overhead. It will require people know ahead of time they want to wait on a subset of items. Is there any other design we should consider?

I wonder if it's worth looking at how other implementations work and how they handle this particular overhead.

@bruno-
Copy link
Contributor

bruno- commented Jul 31, 2022

From #62:

The idea of a barrier is that it's stateful, you can call wait(2) several times until it's exhausted.

Does the current implementation enable this feature? Since @done never changes I think #wait(2) will always return the same result, no?

@zhulik
Copy link
Author

zhulik commented Jul 31, 2022

@bruno- Waiter passes all tests written for Barrier. At least for the case when all task are being awaited. For the case when N tasks are being awainted I decided that it's better when all coroutines waiting for a waiter get same N first results

@ioquatix
Copy link
Member

Closing in favour of #189 as discussed.

@ioquatix ioquatix closed this Oct 31, 2022
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.

3 participants