From 641436c13baaba5fa83ca41e8c09ab8ee2aa6dc2 Mon Sep 17 00:00:00 2001 From: David Harsha Date: Thu, 25 May 2017 00:08:01 -0700 Subject: [PATCH 1/4] Do not execute zipped promises It's unnecessary to immediately execute promises that are chained together with `zip`. This feels like a bug to me, but I'm sure people are relying on the old behavior, so this is definitely a breaking change. --- lib/concurrent/promise.rb | 2 +- spec/concurrent/promise_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/concurrent/promise.rb b/lib/concurrent/promise.rb index 05ef8b431..83ba7ef39 100644 --- a/lib/concurrent/promise.rb +++ b/lib/concurrent/promise.rb @@ -387,7 +387,7 @@ def flat_map(&block) # # @return [Promise] def self.zip(*promises) - zero = fulfill([], executor: ImmediateExecutor.new) + zero = Promise.new(executor: ImmediateExecutor.new) { [] } promises.reduce(zero) do |p1, p2| p1.flat_map do |results| diff --git a/spec/concurrent/promise_spec.rb b/spec/concurrent/promise_spec.rb index 3fb35b276..c60222b93 100644 --- a/spec/concurrent/promise_spec.rb +++ b/spec/concurrent/promise_spec.rb @@ -357,6 +357,12 @@ def get_ivar_from_args(opts) let(:promise2) { Promise.new(executor: :immediate) { 2 } } let(:promise3) { Promise.new(executor: :immediate) { [3] } } + it 'does not execute the returned Promise' do + composite = promise1.zip(promise2, promise3) + + expect(composite).to be_unscheduled + end + it 'yields the results as an array' do composite = promise1.zip(promise2, promise3).execute.wait @@ -375,6 +381,12 @@ def get_ivar_from_args(opts) let(:promise2) { Promise.new(executor: :immediate) { 2 } } let(:promise3) { Promise.new(executor: :immediate) { [3] } } + it 'does not execute the returned Promise' do + composite = Promise.zip(promise1, promise2, promise3) + + expect(composite).to be_unscheduled + end + it 'yields the results as an array' do composite = Promise.zip(promise1, promise2, promise3).execute.wait From 2e8027ab5c47b11f25efe136dd73591417f32a52 Mon Sep 17 00:00:00 2001 From: David Harsha Date: Thu, 25 May 2017 00:42:07 -0700 Subject: [PATCH 2/4] Allow setting executor for zipped promises The use case for this is a little strange, since you can set the executors for the zipped promises when they're created instead. In my case, it would make it easier to run a number of immediately executed promises on a pool: ``` p1 = Concurrent::Promise.new(executor: :immediate) { ... } p2 = Concurrent::Promise.new(executor: :immediate) { ... } ... Concurrent::Promise.zip(p1, p2, executor: SOME_THREAD_POOL).then { ... } ``` --- lib/concurrent/promise.rb | 4 +++- spec/concurrent/promise_spec.rb | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/concurrent/promise.rb b/lib/concurrent/promise.rb index 83ba7ef39..5922d46b4 100644 --- a/lib/concurrent/promise.rb +++ b/lib/concurrent/promise.rb @@ -387,7 +387,9 @@ def flat_map(&block) # # @return [Promise] def self.zip(*promises) - zero = Promise.new(executor: ImmediateExecutor.new) { [] } + opts = promises.last.is_a?(::Hash) ? promises.pop : {} + opts[:executor] ||= ImmediateExecutor.new + zero = Promise.new(opts) { [] } promises.reduce(zero) do |p1, p2| p1.flat_map do |results| diff --git a/spec/concurrent/promise_spec.rb b/spec/concurrent/promise_spec.rb index c60222b93..5225269fb 100644 --- a/spec/concurrent/promise_spec.rb +++ b/spec/concurrent/promise_spec.rb @@ -363,6 +363,14 @@ def get_ivar_from_args(opts) expect(composite).to be_unscheduled end + it 'allows setting executor for Promise chain' do + new_executor = Concurrent::SingleThreadExecutor.new + promise = promise1.zip(promise2, promise3, executor: new_executor) + + promise = promise.instance_variable_get(:@parent) until promise.send(:root?) + expect(promise.instance_variable_get(:@executor)).to be(new_executor) + end + it 'yields the results as an array' do composite = promise1.zip(promise2, promise3).execute.wait @@ -387,6 +395,14 @@ def get_ivar_from_args(opts) expect(composite).to be_unscheduled end + it 'allows setting executor for Promise chain' do + new_executor = Concurrent::SingleThreadExecutor.new + promise = Promise.zip(promise1, promise2, promise3, executor: new_executor) + + promise = promise.instance_variable_get(:@parent) until promise.send(:root?) + expect(promise.instance_variable_get(:@executor)).to be(new_executor) + end + it 'yields the results as an array' do composite = Promise.zip(promise1, promise2, promise3).execute.wait From d85aaf3ab2b274061ba890c3b9932a09233bb168 Mon Sep 17 00:00:00 2001 From: David Harsha Date: Sun, 30 Jul 2017 09:44:42 -0700 Subject: [PATCH 3/4] Add option to not execute zipped promises This restores the default behavior for zipped promises and adds an option (`execute`) to leave them unscheduled. --- lib/concurrent/promise.rb | 8 ++++++-- spec/concurrent/promise_spec.rb | 28 ++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/lib/concurrent/promise.rb b/lib/concurrent/promise.rb index 5922d46b4..a69432fdb 100644 --- a/lib/concurrent/promise.rb +++ b/lib/concurrent/promise.rb @@ -387,9 +387,13 @@ def flat_map(&block) # # @return [Promise] def self.zip(*promises) - opts = promises.last.is_a?(::Hash) ? promises.pop : {} + opts = promises.last.is_a?(::Hash) ? promises.pop.dup : {} opts[:executor] ||= ImmediateExecutor.new - zero = Promise.new(opts) { [] } + zero = if !opts.key?(:execute) || opts.delete(:execute) + fulfill([], opts) + else + Promise.new(opts) { [] } + end promises.reduce(zero) do |p1, p2| p1.flat_map do |results| diff --git a/spec/concurrent/promise_spec.rb b/spec/concurrent/promise_spec.rb index 5225269fb..c745a6c90 100644 --- a/spec/concurrent/promise_spec.rb +++ b/spec/concurrent/promise_spec.rb @@ -357,9 +357,21 @@ def get_ivar_from_args(opts) let(:promise2) { Promise.new(executor: :immediate) { 2 } } let(:promise3) { Promise.new(executor: :immediate) { [3] } } - it 'does not execute the returned Promise' do + it 'executes the returned Promise by default' do composite = promise1.zip(promise2, promise3) + expect(composite).to be_fulfilled + end + + it 'executes the returned Promise when execute is true' do + composite = promise1.zip(promise2, promise3, execute: true) + + expect(composite).to be_fulfilled + end + + it 'does not execute the returned Promise when execute is false' do + composite = promise1.zip(promise2, promise3, execute: false) + expect(composite).to be_unscheduled end @@ -389,9 +401,21 @@ def get_ivar_from_args(opts) let(:promise2) { Promise.new(executor: :immediate) { 2 } } let(:promise3) { Promise.new(executor: :immediate) { [3] } } - it 'does not execute the returned Promise' do + it 'executes the returned Promise by default' do composite = Promise.zip(promise1, promise2, promise3) + expect(composite).to be_fulfilled + end + + it 'executes the returned Promise when execute is true' do + composite = Promise.zip(promise1, promise2, promise3, execute: true) + + expect(composite).to be_fulfilled + end + + it 'does not execute the returned Promise when execute is false' do + composite = Promise.zip(promise1, promise2, promise3, execute: false) + expect(composite).to be_unscheduled end From 0559cbacce1dce2fcde2dc7c7b9f9f166abe1549 Mon Sep 17 00:00:00 2001 From: David Harsha Date: Thu, 21 Dec 2017 11:08:15 -0800 Subject: [PATCH 4/4] Add `executor` and `execute` options to zip docs --- lib/concurrent/promise.rb | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/concurrent/promise.rb b/lib/concurrent/promise.rb index a69432fdb..77a658bbc 100644 --- a/lib/concurrent/promise.rb +++ b/lib/concurrent/promise.rb @@ -383,7 +383,14 @@ def flat_map(&block) # Builds a promise that produces the result of promises in an Array # and fails if any of them fails. # - # @param [Array] promises + # @overload zip(*promises) + # @param [Array] promises + # + # @overload zip(*promises, opts) + # @param [Array] promises + # @param [Hash] opts the configuration options + # @option opts [Executor] :executor (ImmediateExecutor.new) when set use the given `Executor` instance. + # @option opts [Boolean] :execute (true) execute promise before returning # # @return [Promise] def self.zip(*promises) @@ -407,7 +414,14 @@ def self.zip(*promises) # Builds a promise that produces the result of self and others in an Array # and fails if any of them fails. # - # @param [Array] others + # @overload zip(*promises) + # @param [Array] others + # + # @overload zip(*promises, opts) + # @param [Array] others + # @param [Hash] opts the configuration options + # @option opts [Executor] :executor (ImmediateExecutor.new) when set use the given `Executor` instance. + # @option opts [Boolean] :execute (true) execute promise before returning # # @return [Promise] def zip(*others)