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

Support Hash of Futures for Promises.zip #777

Open
f3ndot opened this issue Nov 7, 2018 · 7 comments
Open

Support Hash of Futures for Promises.zip #777

f3ndot opened this issue Nov 7, 2018 · 7 comments
Labels
enhancement Adding features, adding tests, improving documentation. looking-for-contributor We are looking for a contributor to help with this issue.

Comments

@f3ndot
Copy link

f3ndot commented Nov 7, 2018

Reading the guide for the Promises work in 1.1.x I came across the fantastic Concurrent::Promises.zip method.

It's interface, per the guide, expects an array of Futures and will return a final value of said array's values. This is beneficial, but what would make my (and I suspect many other's) code cleaner is to optionally accept a Hash of promises instead of an Array.

e.g.

map_of_work = {
  multiply: Concurrent::Promises.future { 3*2 },
  divide: Concurrent::Promises.future { 3/2 },
  add: Concurrent::Promises.future { 3 + 2 },
  subtract: Concurrent::Promises.future { 3 - 2 },
}
# => {:multiply=>#<Concurrent::Promises::Future:0x00007fc474fad8a0 pending>,
#     :divide=>#<Concurrent::Promises::Future:0x00007fc474fad008 pending>,
#     :add=>#<Concurrent::Promises::Future:0x00007fc474fa7d60 pending>,
#     :subtract=>#<Concurrent::Promises::Future:0x00007fc474fa6ca8 pending>}
Concurrent::Promises.zip(map_of_work).value!
# => {:multiply=>6,
#     :divide=>1,
#     :add=>5,
#     :subtract=>1}

Why? Because it's a common pattern in Javascript libraries[1][2], and thus my team.

Arrays are slightly more cumbersome where I have to remember that index 2 is always for the addition work. Should another developer prepend more Futures to the array, we must not forget to change all the indexes referenced thereafter. With hashes, an :add is always an :add.

@f3ndot f3ndot changed the title Support Hash of Futures for Concurrent::Promises.zip Feature Request: Support Hash of Futures for Promises.zip Nov 7, 2018
@f3ndot
Copy link
Author

f3ndot commented Nov 7, 2018

Forgot to mention that this pattern would apply to the errors section as well:

# ...
Concurrent::Promises.zip(map_of_work).result
# => [true,
#     {:multiply=>6,
#      :divide=>1,
#      :add=>5,
#      :subtract=>1},
#     {:multiply=>nil,
#      :divide=>nil,
#      :add=>nil,
#      :subtract=>nil}]

@pitr-ch pitr-ch added enhancement Adding features, adding tests, improving documentation. looking-for-contributor We are looking for a contributor to help with this issue. medium-priority Should be done soon. high-priority Should be done ASAP. and removed medium-priority Should be done soon. labels Nov 8, 2018
@pitr-ch
Copy link
Member

pitr-ch commented Nov 8, 2018

That is definitely an interesting idea, thanks for sharing! I'll leave it as looking-for-contributor for the moment to see if anybody would be interested to implement it. If not I'll do it.

@pitr-ch pitr-ch changed the title Feature Request: Support Hash of Futures for Promises.zip Support Hash of Futures for Promises.zip Nov 8, 2018
@OunmanNgampeerapong
Copy link

Ok

@jamie
Copy link

jamie commented Jan 30, 2019

So I started working through a POC of this, trying to follow the documentation here for consistency as much as possible. Some notes:

  • Hard to tell the difference between a legit hash and an options hash, resulting alternatives for syntax are all ugly. I think the best is an explicit options hash, but I still hate it: Concurrent::Promises.zip(promise_hash, {})
  • For an initial implementation, I would handle a hash of only promises. No sub-hashes or arrays for values.
  • @f3ndot what do you mean by your second comment? At least as of 1.1.4, .result doesn't exist on Concurrent::Promise, where are you getting yours from?

I'll see about poking at it tomorrow, will post some code once I have a few good specs passing.

@f3ndot
Copy link
Author

f3ndot commented Jan 30, 2019

@jamie remember ::Promise and ::Future are deprecated in favour of ::Promises::Future:

Promises is a new framework unifying former tools Concurrent::Future, Concurrent::Promise, Concurrent::IVar, Concurrent::Event, Concurrent.dataflow, Delay, and TimerTask of concurrent-ruby.

It's a gotcha that I've been bit by a few times.

C:\Users\Justin>gem install concurrent-ruby
Fetching: concurrent-ruby-1.1.4.gem (100%)
Successfully installed concurrent-ruby-1.1.4
Parsing documentation for concurrent-ruby-1.1.4
Installing ri documentation for concurrent-ruby-1.1.4
Done installing documentation for concurrent-ruby after 9 seconds
1 gem installed

C:\Users\Justin>irb
irb(main):001:0> require 'concurrent-ruby'
=> true
irb(main):002:0> Concurrent::Promises.future { 3*2 }
=> #<Concurrent::Promises::Future:0x00000000034356b8 pending>
irb(main):003:0> Concurrent::Promises.future { 3*2 }.result
=> [true, 6, nil]
irb(main):004:0> Concurrent::Promises.future { raise 'oh no' }.result
=> [false, nil, #<RuntimeError: oh no>]
irb(main):005:0>

@jamie
Copy link

jamie commented Jan 30, 2019

Ah, that explains it. I was grepping the code for def self.zip and only found the one in Promise. Concurrent::Promises::FactoryMethods defines zip_futures, and aliases that to zip. That code might be a bit more interesting to refactor hash support into...

@pitr-ch
Copy link
Member

pitr-ch commented Feb 2, 2019

Thanks @jamie for looking into this. Please accept collaboration invitation, then I'll be able to assign the issue to you.

Concurrent::Promises is definitely the right place to add this. The deprecated implementation is maintained but it's not getting any new features.

I am trying to think about how do this flexibly and without too much added complexity to the existing implementation. Something as follows could work

{ a: future { 1 },
  b: future { 2 }
}.reduce(fulfilled_future({})) do |hf, (k, vf)|
  (vf & hf).then { |v, h| h.update k => v }
end.value! #=> { a: 1, b: 2 }

@chrisseaton chrisseaton removed the high-priority Should be done ASAP. label Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation. looking-for-contributor We are looking for a contributor to help with this issue.
Development

No branches or pull requests

5 participants