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

Edge promises fail during error handling #659

Closed
hobodave opened this issue Jun 6, 2017 · 4 comments
Closed

Edge promises fail during error handling #659

hobodave opened this issue Jun 6, 2017 · 4 comments
Labels
bug A bug in the library or documentation.

Comments

@hobodave
Copy link
Contributor

hobodave commented Jun 6, 2017

def normal
  Concurrent::Promises.future { 1 }
end

def fulfilled
  Concurrent::Promises.fulfilled_future(1)
end

def unexpected_failure
  Concurrent::Promises.future { raise "BOOM" }
end

def explicit_failure
  Concurrent::Promises.rejected_future(StandardError.new("BOOM"))
end

def zipped_unexpected
  Concurrent::Promises.zip_futures(unexpected_failure)
end

def zipped_explicit
  Concurrent::Promises.zip_futures(explicit_failure)
end

def zipped_nested
  Concurrent::Promises.zip_futures(zipped_unexpected)
end

normal.value!               # 1
fulfilled.value!            # 1
unexpected_failure.value!   # RuntimeError: BOOM
explicit_failure.value!     # NoMethodError: undefined method `+' for nil:NilClass
zipped_unexpected.value!    # RuntimeError: BOOM
zipped_explicit.value!      # NoMethodError: undefined method `+' for nil:NilClass
zipped_nested.value!        # NoMethodError: undefined method `exception' for [#<RuntimeError: BOOM>]:Concurrent::Array

As shown, the edge promises fail unexpectedly during error handling. The explicit_failure, zipped_explicit, and zipped_nested behavior above all seem like bugs. Note there are two separate issues here:

  1. The behavior of .exception when handling explicitly rejected futures (via .rejected_future)
  2. The behavior of .exception when handling any failure (explicit or unexpected) that is nested within a zip of at least two levels.

Stack traces for each case:

NoMethodError: undefined method `+' for nil:NilClass
	from /Users/davida/.rvm/gems/jruby-9.1.7.0@deal-management-api/gems/concurrent-ruby-edge-0.3.1/lib/concurrent/edge/promises.rb:990:in `exception'
	from /Users/davida/.rvm/gems/jruby-9.1.7.0@deal-management-api/gems/concurrent-ruby-edge-0.3.1/lib/concurrent/edge/promises.rb:1204:in `wait_until_resolved!'
	from /Users/davida/.rvm/gems/jruby-9.1.7.0@deal-management-api/gems/concurrent-ruby-edge-0.3.1/lib/concurrent/edge/promises.rb:975:in `value!'
NoMethodError: undefined method `exception' for [#<RuntimeError: BOOM>]:Concurrent::Array
	from /Users/davida/.rvm/gems/jruby-9.1.7.0@deal-management-api/gems/concurrent-ruby-edge-0.3.1/lib/concurrent/edge/promises.rb:989:in `exception'
	from /Users/davida/.rvm/gems/jruby-9.1.7.0@deal-management-api/gems/concurrent-ruby-edge-0.3.1/lib/concurrent/edge/promises.rb:1204:in `wait_until_resolved!'
	from /Users/davida/.rvm/gems/jruby-9.1.7.0@deal-management-api/gems/concurrent-ruby-edge-0.3.1/lib/concurrent/edge/promises.rb:975:in `value!'

  • JRuby 9.1.7.0
  • concurrent-ruby version: 1.0.5
  • concurrent-ruby-ext installed: no
  • concurrent-ruby-edge used: yes (0.3.1)
@hobodave
Copy link
Contributor Author

hobodave commented Jun 6, 2017

In troubleshooting various possible solutions, I have found one for each.

undefined method `exception' for Concurrent::Array

--- lib/concurrent/edge/promises.rb
+++ lib/concurrent/edge/promises.rb
@@ -981,9 +981,9 @@
      # @raise [StandardError] when raising not rejected future
      # @return [Exception]
      def exception(*args)
        raise Concurrent::Error, 'it is not rejected' unless rejected?
-       reason = Array(internal_state.reason).compact
+       reason = Array(internal_state.reason).compact.flatten
        if reason.size > 1
          Concurrent::MultipleErrors.new reason
        else
          ex = reason[0].exception(*args)

This handles deeply nested errors without issue. Thoughts?

Will update with other solution shortly.

@hobodave
Copy link
Contributor Author

hobodave commented Jun 6, 2017

undefined method `+' for NilClass

--- lib/concurrent/edge/promises.rb
+++ lib/concurrent/edge/promises.rb
@@ -986,9 +986,9 @@
        if reason.size > 1
          Concurrent::MultipleErrors.new reason
        else
          ex = reason[0].exception(*args)
-         ex.set_backtrace ex.backtrace + caller
+         ex.set_backtrace Array(ex.backtrace) + caller
          ex
        end
      end

Ehh. I'm not sure if this is necessary. I think the root problem might be the documentation/examples that show this contrived usage:

Concurrent::Promises.rejected_future(StandardError.new("BOOM"))

Explicitly creating and passing around an exception is odd. Is there any real world usage of this?

The existing code works fine in this example, which is far more real-world:

begin
  raise "BOOM"
rescue => e
  # do something with e, like log or notify or something
  Concurrent::Promises.rejected_future(e)
end

Since this exception was actually raised it will have a non-nil backtrace, and work just fine.

hobodave pushed a commit to hobodave/concurrent-ruby that referenced this issue Jun 6, 2017
* Now properly handles errors raised within deeply nested futures.
* `value!` now properly works with `rejected_future`

Fixes ruby-concurrency#659
@hobodave
Copy link
Contributor Author

hobodave commented Jun 6, 2017

Submitting a PR shortly

hobodave pushed a commit to hobodave/concurrent-ruby that referenced this issue Jun 6, 2017
* Now properly handles errors raised within deeply nested futures.
* `value!` now properly works with `rejected_future`

Fixes ruby-concurrency#659
@pitr-ch pitr-ch added this to the 1.1.0 milestone Jul 23, 2017
@pitr-ch pitr-ch added the bug A bug in the library or documentation. label Jul 23, 2017
@pitr-ch
Copy link
Member

pitr-ch commented Jul 23, 2017

Explicitly creating and passing around an exception is odd. Is there any real world usage of this?

Indeed it's strange, but I think it should still work in this case as you have fixed. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library or documentation.
Projects
None yet
Development

No branches or pull requests

2 participants