Skip to content

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

Merged
byroot merged 1 commit intorails:masterfrom
casperisfine:simplify-promise
Oct 13, 2021
Merged

Return instead of yield from Concurrent::Promise (Ruby 3.1 compat)#720
byroot merged 1 commit intorails:masterfrom
casperisfine:simplify-promise

Conversation

@casperisfine
Copy link
Copy Markdown

@casperisfine casperisfine commented Oct 13, 2021

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.

@byroot byroot merged commit 3aa96f7 into rails:master Oct 13, 2021
@cover cover mentioned this pull request Feb 1, 2022
Closed
@tomstuart tomstuart mentioned this pull request Mar 3, 2022
Closed
@shleeable shleeable mentioned this pull request Mar 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@casperisfine @byroot