ruby-concurrency / concurrent-ruby

Modern concurrency tools including agents, futures, promises, thread pools, supervisors, and more. Inspired by Erlang, Clojure, Scala, Go, Java, JavaScript, and classic concurrency patterns.
https://ruby-concurrency.github.io/concurrent-ruby/
Other
5.68k stars 418 forks source link

`Promises.any_fulfilled_future` does not accept an Event #963

Closed hanazuki closed 1 year ago

hanazuki commented 1 year ago

Documentation on Promises.any_fulfilled_future_on says:

If event is supplied, which does not have value and can be only resolved, it's represented as :fulfilled with value nil.

But actually it fails when you pass an Event to the function.

Reproducing code:

require 'concurrent'
p Concurrent::VERSION

p Concurrent::Promises.any_fulfilled_future(Concurrent::Promises.resolved_event).value!  # => Expect nil

Output:

"1.1.10"
~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:2078:in `resolvable?': undefined method `fulfilled?' for #<Concurrent::Promises::Event:0x00007f672f3cfa30 resolved> (NoMethodError)

        future.fulfilled? ||
              ^^^^^^^^^^^
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:1658:in `on_blocker_resolution'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:808:in `callback_notify_blocked'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:786:in `call_callback'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:751:in `add_callback'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:732:in `add_callback_notify_blocked'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:1637:in `block in new_blocked_by'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:1637:in `each'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:1637:in `each_with_index'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:1637:in `new_blocked_by'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:312:in `any_fulfilled_future_on'
        from ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/promises.rb:299:in `any_fulfilled_future'
        from any_fulfilled_future.rb:4:in `<main>'

* Operating system:                linux
* Ruby implementation:             Ruby
* `concurrent-ruby` version:       1.1.10
* `concurrent-ruby-ext` installed: no
* `concurrent-ruby-edge` used:     no
pitr-ch commented 1 year ago

This should fix it

diff --git a/lib/concurrent-ruby/concurrent/promises.rb b/lib/concurrent-ruby/concurrent/promises.rb
index 76af4d59..eb0c5e2e 100644
--- a/lib/concurrent-ruby/concurrent/promises.rb
+++ b/lib/concurrent-ruby/concurrent/promises.rb
@@ -2074,8 +2074,8 @@ module Concurrent

       private

-      def resolvable?(countdown, future, index)
-        future.fulfilled? ||
+      def resolvable?(countdown, event_or_future, index)
+        (event_or_future.is_a?(Event) ? event_or_future.resolved? : event_or_future.fulfilled?) ||
             # inlined super from BlockedPromise
             countdown.zero?
       end
eregon commented 1 year ago

@hanazuki Could you make a PR with @pitr-ch's patch and add a spec?

hanazuki commented 1 year ago

Done @eregon