Rails5, auto-loading, future, circular dependency

[jrochkind]

My first time trying to use concurrent-ruby in Rails5 and with MRI 2.3. MRI 2.3.1. concurrent-ruby 1.0.2. Running into something very odd.

future = Concurrent::Future.execute { something_that_will_cause_rails_autoloading }
future.value

Hangs forever, never comes back. It's only by some weird debugging that I discovered it's "something_that_will_cause_autoloading" -- and I'm not certain of this, but I suspect it was Rails auto-loading behavior, How? Before calling future.value, I did a weird future.instance_variable_get('@executor').shutdown. After I do that, if I call future.value, it does return, but has failed -- and future.reason is #<RuntimeError: Circular dependency detected while autoloading constant BentoSearch::ResultItem> (which is one of my own classes)

So some thoughts:

1. I am not sure why it's deciding there's a 'circular dependency', I can require 'bento_search/result_item' on it's own (not in a future) fine, with no warnings.
2. I suspect I am doing something Rails doesn't like, perhaps you need to use some kind of eager loading if you are going to use concurrency in Rails 5?
3. But even if I'm doing something wrong, why is the future hanging forever? What's actually going on? I'm not sure. I'd expect it to come back with an exception -- perhaps that "circular dependency" exception, why did I end up with that if I reached in to find the executor and shut it down myself, but a hang forever otherwise? It seems like concurrent-ruby is maybe doing something wrong.... maybe?

Wow, concurrency is confusing. Any thoughts.

[pitr-ch]

@jrochkind Thanks for letting us know! We definitely pay attention to rails + c-r combination. Could you create a reproducer for this? The blocking part is enough.

[jrochkind]

Upon further investigation, I think it is about Rails auto-loading, and is all about Rails, and not at a ruby-concurrent bug or something ruby-concurrent can do anything about.

But is something users of ruby-concurrent in a Rails app will have to know about, that you may want to give them hints on or be able to answer questions on.

Repro with giant write-up here, also reported as Rails issue.

Basically, using concurrency in the request-loop in Rails5 seems to require turning off dev-mode class reloading, and the 'failure mode' if you fail to do so looks like a deadlock.

[jrochkind]

Okay, I've figured some stuff out by reading source, expirimentation, but mostly by @matthewd answering my questions.

Rails 5 has several new architectural components and API for dealing with concurrency.

The good news is that can make auto-loading, and even dev-mode reloading, concurrency-safe for the first time ever.

The bad news it's pretty much entirely undocumented (not even method-level rdoc mostly), and if you don't use it correctly the 'failure mode' can be a deadlock in your application. As opposed to Rails4, where autoload concurrency failure meant weird exceptions, now it can often mean deadlock.

The 'correct' way to do some concurrency inside a Rails action method and then wait for the work to complete before returning a response:

  # a controller action method
  def show
    futures = 3.times.collect do |i|
      Concurrent::Future.execute do
        Rails.application.executor.wrap do
          SomeWorker.new.value
       end
      end
    end

    ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
      @example_values = futures.collect(&:value)
    end
  end

• ActiveSupport::Dependencies.interlock.permit_concurrent_loads

  In cases where autoloading happens (an actual autoload is triggered, an autoloadable constant not yet loaded is accessed), if you don't do this, you wind up with deadlock, where the threads are trying to autoload, but can't, because the Rails request/action/worker has some type of lock on autoloading. The future threads are waiting for the lock to be relinquished, but the worker thread with the lock is waiting for the threads to be complete.

  So in those cases, the threads will actually be blocked until we do permit_concurrent_loads, at which point they'll be allowed to do their thing, and we wait for them to complete inside that block.

  If autoloading isn't happening, this isn't necessary. So for instance, if Rails config.eager_load == config.cache_classes == true (as normally in production, but not dev), then I don't think it's neccesary.

  It probably doesn't hurt even when used where not necessary.

• Rails.application.executor.wrap

  Rails new API for signaling intent 'to do some work'. One thing it does is make autoloading actually work concurrency-safe. If you don't have this line here, and autoloading happens inside the future (actually is triggered, not just an auto-loadable constant, but one which has not yet been loaded), then one or more futures may raise RuntimeError: Circular dependency detected while autoloading constant NameOfClass. I am not sure why this exhibits with a message "Circular dependency" or what's "circular" about it, but it means "you didn't autoload in a concurrency safe way".

  Rails.application.executor.wrap around your work is the way to make autoloading concurrency safe. But it's intended to be used regardless of dev or production mode, it's always intended to be used around 'Rails-related work being done' in a thread.

  It may eliminate the need for ye old ActiveRecord::Base.connection_pool.with_connection, the wrap takes care of checking connections back in at the end of the block for you. Needs more verification.

  There's another version, Rails.application.reloader.wrap. I believe the functionality there is a superset of Rails.application.executor.wrap, but I'm not totally sure in what ways. But you can't use it in this case, since a Rails request loop is already wrapped in a Rails.application.reloader.wrap, using one here... seems to result in deadlock. Sidekiq does use it for it's workers, which I think result in the workers actually getting dev-mode reloading working for them. So that's one extra thing it does.

  If you are in 'production-mode' configuration (usually no auto-loading) what are the consequences of not using Rails.application.executor.wrap? Not sure.

Phew. I haven't fully wrapped my head around how this impacts using gems which use concurrency (with concurrent-ruby or otherwise) that aren't neccessarily written specifically for Rails, so don't have any Rails specific API in them and don't want any -- if they end up triggering any Rails auto-loading, it could be bad and deadlock. They probably wouldn't ordinarily, unless you pass class names into them or something.

Removing certain directories from Rails config.autoload_paths might be one solution, I haven't fully investigated it. It's unclear to me if config.autoload does anything in Rails5.

@matthewd promises some actual docs, which I eager await. If anyone else wants to play around with concurrent-ruby in a Rails app, your feedback or problems you run into might help @matthewd understand his audience and what the docs need to hit.

[jrochkind]

So, yeah, it's not a concurrent-ruby issue... but I think many many people trying to use concurrent-ruby in Rails5 are going to run into these issues, so concurrent-ruby team probably wants to be aware of them, and maybe even doc them especially if Rails doesn't doc them soon.

[ianks]

I stumbled across this issue as well. Spend an afternoon trying to figure out why I started deadlocking. I think dealing with this should be high-priority since it really highers the burden of entry on using concurrent patterns in Rails.

To put this in perspective a little bit, I (as a contributor to this library who actively enjoys studying concurrency), spent hours tracking this bug down. Imagine the pain others may feel.

Let's start brainstorming some solutions:

1. Disable multithreading if Rails.env.development? (not a good solution at all IMO)
2. If a user deadlocks, or some circular dep is detected, give them a specific warning and references about how to fix the issue (better)
3. Always permit_concurrent_loads somehow when blocking CR methods are running (meh... this would still cause auto-load race conditions to occur, but is arguably better than getting stuck on deadlocks).

All in all, this is somewhat leading me to believe that we should create a rails integration gem concurrent-ruby-rails which attempt to address many of these Rails specific concerns.

[pitr-ch]

yeah that's a good idea, I was thinking that we should have a bridge/adapter to make c-r work easily under Rails. However I am no longer actively working with Rails applications and I do not have time to do it. I'll support anyone (and help when I can) who would decide to take up the task. @ianks ?

[jrochkind]

I'm honestly not sure there's any good way to take care of it for the user, the Rails architecture may just need the user to express their intent as intended. Good docs supplementing Rails docs may be the best that can be done.

[pitr-ch]

I was hoping to wrap c-r tasks with permit_concurrent_loads or something like that. For promises it might be easy, theirs FactoryMethods were designed to support cases like that. Other abstractions might be problematic. I did not thing about it in depth at all. Even if the gem provides only few key abstractions which will work with Rails it might be a big win for users.

[matthewd]

I'm happy to provide information on what Rails needs etc.

I've given up on addressing it myself -- all my attempts have been vetoed because they involve providing APIs that a sufficiently-misguided library could misuse, and I still see no way around that.

[ianks]

It would be nice to have a blessed way of doing concurrency inside of the Rails request life cycle. I'm finding myself commonly wanting to resolve from external APIs, and I imagine it's a common use case. I've done alright for myself building abstractions which do the Rails.application.executor dance, but I think it's error-prone and tedious.

@matthewd, do you think a proposal for something like ActiveSupport::Concurrency::Promise would be considered? Or would this be something that should be third party?

[edwardmp]

@jrochkind

Thanks for your write-up. It helped me tremendously when I encountered the 'circular dependency detected while autoloading constant NameOfClass.' issue. I don't know much about threading in Ruby in general.

Here are my observations:

• Not using Rails.application.executor.wrap sometimes leads to the circular dependency issue. Actually in most cases it did not, but it still happened from time to time.
• After using just Rails.application.executor.wrap my process went into a deadlock.
• After calling ActiveSupport::Dependencies.interlock.permit_concurrent_loads at the end of the future execution resolves the deadlock. And yes, config.cache_classes is set to true (production) but I do actually need this (you thought you wouldn't need it with config.cache_classes enabled).

With regard to the last issue, if I look at your example the permit_concurrent_loads is not called from within the future execution cycle. So although this works, I am wondering if this makes sense?

Here's a bit of sample code:

pool = Concurrent::FixedThreadPool.new(Concurrent.processor_count * 2)

# Process jobs from tube
@beanstalk.jobs.register(tube_name) do |job|
    ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
      @logger.info "Received job: #{job.id}"
      Concurrent::Future.execute(executor: pool) do
        Rails.application.reloader.wrap do
          process_beanstalk_message(job.body)
        end
      end
    end
end

@beanstalk.jobs.process!(reserve_timeout: 10)

[jrochkind]

@edwardmp that's odd that you need this apparatus in production, and disturbing to me. I don't understand what's going on. Just to check, you say you have cache_classes=true, do you also have config.eager_load = true (as would be standard/default in production)? @matthewd, any ideas or explanation of why you'd need to use the executor and interlock objects to do concurrency even in production configurations?

[jrochkind]

So a naive-implementation of a Rails-safe Future, for instance, might just be one that always wraps it's body in Rails.application.executor.wrap, and wraps value in ActiveSupport::Dependencies.interlock.permit_concurrent_loads.

I'm not sure of the performance implications.

If you are not in a situation that requires this (I thought if you are in prod-mode configurations without autoloading it should not be needed, but may not be true according to reports above), are you paying a non-trivial expense?

How expensive is ActiveSupport::Dependencies.interlock.permit_concurrent_loads in cases where nothing is going to end up autoloaded anyway, are you paying a non-trivial cost by wrapping each individual value in it?

@matthewd can you give any tips or guidance on the performance considerations? And I know Rails.application.executor.wrap is re-entrant -- I think ActiveSupport::Dependencies.interlock.permit_concurrent_loads should be as well?

One could imagine a rails-safe implementation of (eg) Future that actually feature-checked to make sure the relevant methods were available, so you could use it in any version of Rails and non-Rails as well.

[jdantonio]

@pitr-ch We haven't heard from you in a while? Are you still interested in maintaining this gem? I haven't worked with Ruby at all in the past two years and don't expect that to change in the foreseeable future. But many people depend on this gem so we need to make sure it's well maintained.

[jrochkind]

Uh oh, it's a dependency of Rails now too, it def needs to be well-maintained. It's whole raison d etre IMO is being well-maintained standard shareable concurrency primitives, the existence of which will allow ruby open source to go much further with concurrency and work together with the same primitives.

I would not assume not commenting on this issue (which is Rails-specific, and concurrent-ruby is not) means a lack of maintenance. But if there is a lack of maintenance, it would be a problem to many people's plans, and should be made transparent and dealt with. This thing existing is hugely useful to me and many others, but only if it's maintained.

[jdantonio]

I stepped down as maintainer in October of last year (issue #580), but I still follow notifications. If @pitr-ch isn't interested in maintaining the gem anymore I'll step up my involvement for a bit to help with these outstanding issues. But the best long-term solution is for someone who works regularly in Ruby to take the lead.

[matthewd]

@jrochkind right, that's exactly what I wanted to do: allows Rails (or any other interested library) to automatically wrap futures when it is necessary to ensure [high-concurrency] safety.

The cost should be minimal (and if it's not, we can and should fix that in Rails)... and should only be paid if it's needed, in which case the cost is arguably immaterial anyway.

The fact @edwardmp is seeing runtime autoloading at all would imply that eager_load is disabled, so that should settle your disturbance there.

As I mentioned in March, I really wanted/want to address this transparently in Rails so it'll Just Work for everyone. But @pitr-ch has made it clear he's not prepared to provide the API we need to do that, because some other library might misuse it; and I'm really not keen on the idea of monkey-patching c-r to achieve it.

[jrochkind]

Thanks @matthewd. I'm confused, is @pitr-ch a Rails committer too?

What were your thoughts/plans for how to do this transparently? Whether it's part of rails, part of c-r, an extra gem, a monkey-patch, whatever -- you're the expert here, and I think the fact that this stuff doens't Just Work in a Rails environment is a real barrier to increasing use of concurrency in places it makes sense in Rails-land, and I'd like to figure out what's up and see if we can provide some solutions.

It does not seem totally appropriate to have c-r itself referring to Rails classes like Rails.application.executor.wrap -- I doubt that was your thought. What was your thought? If you wanted to share an outline, it would be helpful to many of us I think.

[jdantonio]

I'm not familiar with the issue or its history so I'll have to catch up before I can express an opinion. I'm probably less concerned about people misusing a feature than @pitr-ch is. This is a concurrency library after all. Proper use of the more advanced featured requires a certain level of knowledge to begin with. I don't see why we can't work something out.

[chrisseaton]

I can spend a couple of hours a week trying to get the issue count down in general.

[matthewd]

My plan involves c-r providing a low-level API (which Rails would call), to say essentially "please wrap every future-execution with this block" (the executor), and possibly another to wrap the value-wait (the permit). It's not elegant, but it's the only Works By Default option that I can see -- I'm certainly open to alternative suggestions, though.

---

The first time I brought it up, @pitr-ch threatened to remove the default executors ("This discussion helped me realise [the current API is too convenient]"), and made it clear he was using his maintainership of this library to "keep advocating against" (potentially questionable, but very present in the language) core ruby design concerns.

The second time, he implied I (or, my requested API) was disrespecting / not "appreciating" c-r's users, and said he would not knowingly support a situation where c-r provided an API that two libraries could use in a conflicting manner, because it might happen and that's a good enough reason to disallow it.

It was about there that I gave up.

[chrisseaton]

@matthewd knowing @pitr-ch well I'm sure he's trying to do the right thing and is open to ideas and compromises and any bad feeling here is accidental. Please don't give up communicating what you need. I'll try to make time to read back through this issue and see what the issues are as well.

[jrochkind]

I think it would be worth revisiting with more maintainers/committers involved.

But if it can't/won't be made part of c-r, the options are:

1. monkey-patching c-r
2. providing 'wrapper' classes in another gem that proxy to c-r.

I think it's worth considering either, if neccesary. We need a way to do concurrency in rails that isn't a mess. Sadly/ironically/paradoxically, @matthewd's very clever improvements to Rails 5.1 which make it possible to do auto-loading in a reliably concurrency-safe way for the first time -- also mean you need to use Rails API when doing any concurrency, or you get a deadlock (if there's any auto-loading going on). Which is a big problem, and makes it a heck of a lot harder to do concurrrency. Before, you'd get occasional race conditions you could just ignore if you had auto-loading on--now you get reproducible deadlocks instead, if you don't use Rails api.

While I understand and appreciate the intent, I'm not super happy with this outcome. Even if we fix c-r to work somehow, there are libraries and people doing multi-threaded concurrency without c-r too. But anyways.

Ideally there should be a way not only to easily write concurrent stuff for Rails, but a way to use concurrent stuff written not neccesarily with rails in mind safely in rails as well. With regard to c-r, that probably means a monkey-patch rather than a proxy class... I think probably. I'm willing to consider it as a developer-user.

[jrochkind]

@matthewd btw, something that would still really help is still docs... the docs in this 10-month-old un-merged PR are actually super helpful, it would be nice if they were more discoverable than they are in an un-merged PR. https://github.com/rails/rails/pull/27494

Anyone on this thread trying to figure out what's we're talking about in Rails, I think the docs in that un-merged PR are probably the best/only overview.

[jdantonio]

All: I emailed @pitr-ch on his private email. He says he wants to be the maintainer of this gem, admits he hasn't been very responsive, and has promised to talk to me more over the next few days. I've worked with him on this gem for several years and I know he cares very deeply about its success. Please give us a chance to talk things over and figure out what's best for everyone. Unless there is an immediate need, can we revisit this after the American Thanksgiving holiday?

[pitr-ch]

Hi sorry, I was very busy over the summer and I let myself ignore notifications from c-r. However I did not intend to misconfigure my filters for mentions, I thought nobody was looking for me personally. I'll catch up, and please accept my apologies.

[edwardmp]

@jrochkind

Thanks for your response. My settings relating to autoload should be pretty default. Indeed in production cache_classes = true and eager_load = true.

That being said, this issue happens pretty infrequently in my production environment, the last occurrence was about 50 days ago. I currently do not wrap my code using stuff like Rails.application.executor.wrap anymore.

CC @matthewd TLDR; oddly enough eager_load is not disabled, so that's not what's happening in my case, very strange.

[matthewd]

@edwardmp maybe you have entries in autoload_paths that aren't in eager_load_paths? :confused:

If everything is eager loaded, there shouldn't be anything left to autoload.

[jrochkind]

@matthewd this is kind of verging to another topic but it's something I have not been able to figure out (and it is relevant to using rails with concurrency safely) -- is there any way to actually turn off autoloading in Rails? I have not been able to find it in any guides or docs, and have not been able to figure out the code. Like even if you have entries in autoload_paths that aren't in eager_load_paths, just say "no, don't do autoloading" -- cause I'd rather get a clear "constant not found" error than a deadlock.

(Also makes one think it should not be allowed to have things in autoload_paths that aren't in eager_load_paths, rails should not allow it. The consequences of doing something weird here get more drastic in rails 5.1, and this has always been a confusing high-churn part of Rails).

[matthewd]

Yes: http://guides.rubyonrails.org/upgrading_ruby_on_rails.html#autoloading-is-disabled-after-booting-in-the-production-environment

Also part of my confusion about how @edwardmp is getting far enough into the autoloader to see the "circular dependency detected" message.