Skip to content

Conversation

@amutake
Copy link
Contributor

@amutake amutake commented Apr 10, 2015

I found deadlock situation in using Future when the handler raises Exception (not StandardError).

require 'concurrent'

future = Concurrent::Future.new do
  raise Exception
end
future.execute
future.value #=> deadlock
/Users/shohei-yasutake/work/concurrent-ruby/lib/concurrent/atomic/condition.rb:50:in `sleep': No live threads left. Deadlock? (fatal)
    from /Users/shohei-yasutake/work/concurrent-ruby/lib/concurrent/atomic/condition.rb:50:in `wait'
    from /Users/shohei-yasutake/work/concurrent-ruby/lib/concurrent/atomic/condition.rb:50:in `wait'
    from /Users/shohei-yasutake/work/concurrent-ruby/lib/concurrent/atomic/event.rb:89:in `wait'
    from /Users/shohei-yasutake/work/concurrent-ruby/lib/concurrent/obligation.rb:80:in `wait'
    from /Users/shohei-yasutake/work/concurrent-ruby/lib/concurrent/obligation.rb:71:in `value'
    from example.rb:7:in `<main>'

It is because the exception raised in SafeTaskExecutor.new(@task).execute(*@args) is not caught and complete(success, val, reason) is not called.

https://github.com/ruby-concurrency/concurrent-ruby/blob/master/lib/concurrent/future.rb#L85-L86

So I fixed to catch Exception in SafeTaskExecutor.

@amutake amutake force-pushed the fix-future-deadlock branch from 671e059 to 56209f8 Compare April 10, 2015 05:07
@pitr-ch
Copy link
Member

pitr-ch commented Apr 10, 2015

Thanks for reporting and for the fix!

Copy link
Member

Choose a reason for hiding this comment

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

Could you also test that future.reason is set to the Exception instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for quick response!

I added a test case. Could you check it?

* Check that future.reason is set to the Exception instance
  when the handler raises Exception (not StandardError)
@jdantonio
Copy link
Member

@amutake Thank you very much for catching this and taking the time to fix it! I'll take a look at this in more detail this evening. Hopefully we can get it merged today.

jdantonio added a commit that referenced this pull request Apr 19, 2015
Fix deadlock of Future when the handler raises Exception
@jdantonio jdantonio merged commit b9d9469 into ruby-concurrency:master Apr 19, 2015
This was referenced Apr 19, 2015
@pitr-ch pitr-ch added this to the 0.9.0 Release milestone Apr 23, 2015
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