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

Does not work well with async gem #214

Closed
morgoth opened this issue Mar 21, 2022 · 11 comments
Closed

Does not work well with async gem #214

morgoth opened this issue Mar 21, 2022 · 11 comments

Comments

@morgoth
Copy link

morgoth commented Mar 21, 2022

I integrated async into the Rails app and I noticed issues with using this gem and Rails autoloder (powered by zeitwerk).

I have the sample app that presents the bug - the setup is visible in this commit morgoth/async-autoload@ed3a76c

So the problem is that when autoloading some class inside the async tasks, the class is properly loaded only once and all other invocations are failed.

As simple as:

Async do |task|
  3.times do |i|
    task.async do
      MyModel.run_something # Problems with autloading
    rescue => e
      puts e
    end
  end
end.wait

I tried to produce a simple Zeitwerk script to present a failure, but I couldn't so somehow I can reproduce it in the Rails app only.

It can be workaround by manually requiring the file that should be autoloaded or by setting config.eager_load = true in the Rails app.

I guess the "async" gem is the simplest way to parallelize something, but using it within the Rails app causes such problems, so it might not be that easy to popularize it in the Rails community.

@fxn do you have any clue if this could be somehow fixed or is it a known limitation?

FYI @ioquatix @bruno-

My env:
ruby 3.1.1p18
async-2.0.1
rails-7.0.2.3
zeitwerk-2.5.4

@fxn
Copy link
Owner

fxn commented Mar 21, 2022

Interesting. If it is an issue with Zeitwerk we should be able to reproduce in a minimal script without Rails. Things that come to mind are guarantees of async wrt concurrent require, since Module#autoload is basically a delayed require.

(If this was not a Zeitwerk or async issue, then we'd close here and open a ticket in Rails.)

@morgoth
Copy link
Author

morgoth commented Mar 21, 2022

I tried:

gem "zeitwerk"
gem "async"

require "zeitwerk"
require "async"

loader = Zeitwerk::Loader.new
loader.push_dir("scripts")
loader.setup

Async do |task|
  5.times do |i|
    task.async do
      Runner.run # Defined in scripts/runner.b
      sleep rand
    end
  end
end.wait

but it works fine

@fxn
Copy link
Owner

fxn commented Mar 21, 2022

This script reproduces what Zeitwerk essentially does in your behalf with pure Ruby:

require 'tempfile'
require 'async'

Tempfile.create(['foo', '.rb']) do |file|
  file.write(<<~RUBY)
    class C
      def self.call_me
      end
    end
  RUBY
  file.close

  autoload :C, file.path

  Async do |task|
    5.times do |i|
      task.async do
        p C.call_me
        sleep rand
      end
    end
  end.wait
end

I cannot see it fail.

@bruno-
Copy link

bruno- commented Mar 22, 2022

Async 2 had some tempfile issues: socketry/async#138 (comment) , and socketry/async#151. That is apparently fixed with io-event gem version 1.0.5 (dependency of async).

@morgoth maybe ensure io-event gem is 1.0.5 and see if the error happens again?
Btw. I assume you're on Linux?

@morgoth
Copy link
Author

morgoth commented Mar 22, 2022

@bruno- it is 1.0.5 as you can see here https://github.com/morgoth/async-autoload/blob/master/Gemfile.lock#L89
Yes, I'm on Linux.

@fxn Should I close the ticket here and open one on the rails repo?

@ioquatix
Copy link

I wonder if there is a mutex in the autoloading part of zeitwerk that is making some assumptions about thread or fiber locals.

@fxn
Copy link
Owner

fxn commented Mar 23, 2022

Autoloading is done by Ruby, Zeitwerk inherits thread-safety from Ruby and does not have a custom lock around it.

That is, while CRuby is autoloading class C via Module#autoload, there's no context switch in the middle of it. So, no other code concurrently accessing C sees a partially defined class.

See this test.

There's a lock for autovivification of implicit namespaces only.

@fxn
Copy link
Owner

fxn commented Mar 23, 2022

@morgoth I'd close here and open an issue in the Rails repo:

  • We have never seen the scripts above fail.
  • The equivalent code in the Rails test you provided always fail.

Therefore, I believe we have to work with the hypothesis that Zeitwerk and Async themselves are fine, and the issue appears when you mix in Rails.

@fxn
Copy link
Owner

fxn commented Mar 23, 2022

I have reproduced with bin/rails runner too. It is consistent, if you throw

require "async"

unless Object.autoload?(:MyModel)
  autoload :MyModel, "#{__dir__}/app/models/my_model.rb"
end

Async do |task|
  10.times do |i|
    task.async do
      MyModel.run_something
    rescue
      p $!
    end
  end
end.wait

in the root directory, CRuby consistently runs it fine, and bin/rails runner does not.

@morgoth
Copy link
Author

morgoth commented Mar 23, 2022

Ok, I opened the Rails issue rails/rails#44751
Let's move the discussion there.
Thank you for investigation

@morgoth morgoth closed this as completed Mar 23, 2022
@ioquatix
Copy link

Thanks for the repro, I'll also see if it's something obvious.

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

No branches or pull requests

4 participants