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

Wait for all threads to finish on parallel runner #535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion lib/sshkit/runners/parallel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,24 @@ def execute
end
end
end
threads.each(&:join)

wait_for_threads(threads)
end

private

def wait_for_threads(threads)
exception = nil
Copy link

Choose a reason for hiding this comment

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

Perhaps storing all of the exceptions would be more informative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be a nice improvement - if something fails on multiple hosts it can be tempting to assume the problem is specific just to the host that the error is reported on.

We'd need a way to do it without breaking the current API though which is an ExecuteError with a single cause. We could store multiple causes I guess and have ExecuteError#cause return the first one?

But I think that would be something for another PR.


threads.map do |t|
begin
t.join
rescue ExecuteError => e
exception ||= e
end
end

raise exception if exception
end
end

Expand Down
25 changes: 25 additions & 0 deletions test/unit/runners/test_parallel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,31 @@ def test_wraps_ruby_standard_error_in_execute_error
assert_match(/deployer@example/, error.message)
assert_match(/oh no!/, error.message)
end

def test_waits_for_all_threads_to_finish_on_error
hosts = [Host.new("deployer@example"), Host.new("deployer@example2"), Host.new("deployer@example3")]
completed_one, completed_three = false, false
runner = Parallel.new(hosts) do |host|
case host.hostname
when "example"
sleep 0.1
completed_one = true
when "example2"
raise "Boom!"
when "example3"
sleep 0.3
completed_three = true
end
end

error = assert_raises(SSHKit::Runner::ExecuteError) do
runner.execute
end
assert_match(/deployer@example2/, error.message)
assert_match(/Boom!/, error.message)
assert completed_one
assert completed_three
end
end
end
end