rescue Exception in Futures #431
Conversation
pitr-ch
added
the
enhancement
| complete_with Future::Failed.new(error) | ||
| rescue Exception => error | ||
| complete_with Future::Failed.new(error) | ||
| log(ERROR, 'Edge::Future', error) |
There was a problem hiding this comment.
Doesn't this change the return value of this method when rescuing Exception? It appears to no longer return a Future. I'm not sure of the ramifications.
There was a problem hiding this comment.
Nice catch, it would break the documented behaviour, it would break CompletableFuture#evaluate_to. Fixing and merging.
|
FYI, we're using this in our code, and it's working. So, once my comment above is addressed (if needed) this is good to merge. 👍 |
|
@hobodave Do you mind if I ask what project you are using this in? I 💚 hearing about projects that use this gem! |
|
@jdantonio I'm using it in a REST API we're building internally. The API is basically serving as an aggregator of several other internal APIs. So, one request to our application may need to make dozens (or hundreds in rare cases) of concurrent requests to downstream APIs to return a response in a timely fashion. |
|
@jdantonio Any chance you could build 1.0.0pre4/0.2.0pre4 incorporating this fix? We're unable to bundle/use the gem when using the git/ref syntax in the Gemfile. Thanks! |
|
Certainly. I'll try to get that out in the next day or two. Will that be soon enough? (I needed to get the pre3 update out ASAP for Rails, which is why I didn't wait for this PR.) |
|
what is the problem with git/ref ? |
|
@jdantonio That's fine. @pitr-ch It's a really bizarre error. I thought it might be my local environment, but another developer on my team is having the same issue on his local machine. Here's the details: # Gemfile
gem 'concurrent-ruby', git: 'https://github.com/ruby-concurrency/concurrent-ruby.git', ref: '1b24e0bd48e7bdd0b6b1c98820d2f470296df755'
gem 'concurrent-ruby-edge', git: 'https://github.com/ruby-concurrency/concurrent-ruby.git', ref: '1b24e0bd48e7bdd0b6b1c98820d2f470296df755'When attempting to run rspec via console: This only happens when I point directly at a ref. Strangely, if I point directly at my local copy: # Gemfile
gem 'concurrent-ruby', path: '/Users/davida/Developer/concurrent-ruby'
gem 'concurrent-ruby-edge', path: '/Users/davida/Developer/concurrent-ruby'This works just fine. Should I create an issue for this? |
|
@hobodave Please open an issue, if you don't mind. I've taken a cursory look at the code and I think the problem is the compiled Java extensions. If you compiled the Java extensions in your local repo then they will load when you point your Gemfile to the local path, but they won't ever load when you reference the git repo (since we don't check the compiled Java code into git). If I'm correct you can verify by deleting the compiled code from your local repo and see if the error surfaces. |
|
@pitr-ch I think the problem is on line 9 of object.rb. When on JRuby we assume that the Java extensions are compiled and loaded. On line 45 of native_extension_loaded.rb we silently fail if the Java extensions do not load, so our assumption is not valid. Contrast this to how we load the C extensions on line 84 of atomic_boolean.rb. In the latter case we use One way to handle this would be to allow the load error in native_extension_loader.rb to bubble up. I'm not a big fan of this approach. A JRuby user should be able to use the pure-Ruby gem. We should probably log something in native_extension_loader.rb rather than fail silently. Another approach would be to use ObjectImplementation = case
when Concurrent.on_cruby?
MriObject
when defined? JRubyObject
JRubyObject
when Concurrent.on_rbx?
RbxObject
else
warn 'Possibly unsupported Ruby implementation'
MriObject
endI'll create a PR with these changes then we can move the discussion over there. |
|
See PR #434. Let's move discussion of the load error to that thread. |
follow up of #430