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

Return instead of yield from Concurrent::Promise (Ruby 3.1 compat) #720

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

casperisfine
Copy link

So i have to admit I don't fully understand what's going nor how it even worked until now.

While testing ruby 3.1.0-dev on a Rails app using sprockets 4.0.2, I get the following error:

Minitest::UnexpectedError: ActionView::Template::Error: undefined method `exception' for nil:NilClass
 
        reason.exception(*args)
              ^^^^^^^^^^
    /tmp/bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:128:in `exception'
    /tmp/bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `raise'
    /tmp/bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `block in wait!'
    <internal:kernel>:90:in `tap'
    /tmp/bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `wait!'
    /tmp/bundle/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:130:in `each'
    /tmp/bundle/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:130:in `find'
    /tmp/bundle/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:142:in `each'
    /tmp/bundle/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:142:in `find_sources'

I dug into concurrent-ruby and somehow the wait! on the promise is supposed to return multiple values, but simply return nil.

I think it's caused by the fact that we yield when we're supposed to return, so we're somehow interupting the flow.

But looking at the original code I don't even get how it's supposed to work in the first place:

    def find(*args)
      unless environment
        raise Error, "manifest requires environment for compilation"
      end

      return to_enum(__method__, *args) unless block_given?

      environment = self.environment.cached
      promises = args.flatten.map do |path|
        Concurrent::Promise.execute(executor: executor) do
          environment.find_all_linked_assets(path) do |asset|
            yield asset
          end
        end
      end
      promises.each(&:wait!)

      nil
    end

The block passed to Concurrent::Promise.execute executes in a background thread, so there's no-one to yield to.

What's weird is that the thread is like immediately killed, there's no backtraces or anything, so that might be a Ruby bug of some sort.

Either way, we should return rather than yield.

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.

2 participants