{"action":"created","issue":{"url":"https://api.github.com/repos/rails/rails/issues/26847","repository_url":"https://api.github.com/repos/rails/rails","labels_url":"https://api.github.com/repos/rails/rails/issues/26847/labels{/name}","comments_url":"https://api.github.com/repos/rails/rails/issues/26847/comments","events_url":"https://api.github.com/repos/rails/rails/issues/26847/events","html_url":"https://github.com/rails/rails/issues/26847","id":184306219,"number":26847,"title":"Rails5, concurrency in action method, deadlock, docs needed including for executor/reloader","user":{"login":"jrochkind","id":149304,"avatar_url":"https://avatars.githubusercontent.com/u/149304?v=3","gravatar_id":"","url":"https://api.github.com/users/jrochkind","html_url":"https://github.com/jrochkind","followers_url":"https://api.github.com/users/jrochkind/followers","following_url":"https://api.github.com/users/jrochkind/following{/other_user}","gists_url":"https://api.github.com/users/jrochkind/gists{/gist_id}","starred_url":"https://api.github.com/users/jrochkind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrochkind/subscriptions","organizations_url":"https://api.github.com/users/jrochkind/orgs","repos_url":"https://api.github.com/users/jrochkind/repos","events_url":"https://api.github.com/users/jrochkind/events{/privacy}","received_events_url":"https://api.github.com/users/jrochkind/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/rails/rails/labels/actionpack","name":"actionpack","color":"FFF700"},{"url":"https://api.github.com/repos/rails/rails/labels/needs%20feedback","name":"needs feedback","color":"ededed"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2016-10-20T18:32:08Z","updated_at":"2016-10-22T00:44:09Z","closed_at":null,"body":"I sometimes need/want to use some basic threaded concurrency inside a rails action --\r\nexecute some things concurrently, wait for them all to complete before returning\r\nthe response.\r\n\r\nHere is a basic isolated fake example, as used in this demo app:\r\n\r\n~~~ruby\r\n  # action method\r\n  def example\r\n    futures = 3.times.collect do\r\n      Concurrent::Future.execute do\r\n          SomeWorker.new.value\r\n        end\r\n    end\r\n\r\n    @example_values = futures.collect(&:value)\r\n  end\r\n~~~\r\n\r\nThis is a useful thing to do you really do need to wait for those \"SomeWorker\"\r\nresults to return a response, but they are a bit slow, and can be performed\r\nconcurrently/in parallel. (Even with MRI GIL this is effective when they are\r\nslow because of IO).\r\n\r\nIn Rails 4.x, under default configuration in an app, in development mode,\r\nthis would _mostly_ work fine, but would _occasionally_ raise a weird\r\nexception involving \"class definition has changed\" or something, I'm afraid I\r\nforget the exact exception class/message and can't find it.\r\n\r\nIn Rails 4.x, if you correct set up the under-documented and inter-related\r\nrelevant config keys --  `config.eager_load`, `config.auto_load` and\r\n`config.cache_classes` -- you could make it work entirely correctly, probably\r\nby giving up development-mode class reloading though.\r\n\r\nIn Rails 4.x, it works fine under default production configuration, and as\r\nfar as I know does in Rails 5 too.\r\n\r\nBut in development mode, tn Rails 5.0, due to new autoloading logic, the \"failure mode\" has changed --\r\nif you don't configure things correct, instead of (Rails 4) getting mostly working\r\nbut occasionally weird exception -- you get (Rails 5) the worker thread hanging\r\nforever (presumably a deadlock).\r\n\r\n[My reproduction/demonstration app](https://github.com/jrochkind/rails5_concurrency_autoload_repro) has `config/development.rb` set up to use ENV vars\r\nto make testing under various config combinations more convenient.\r\n\r\nFindings:\r\n\r\n1. Under default generated app config\r\n\r\n        config.cache_classes = false\r\n        config.eager_load = false\r\n        config.auto_load => not explicitly set\r\n\r\n   Rails worker thread hangs forever on first request.\r\n\r\n\r\n2. CONF_EAGER_LOAD=false CONF_CACHE_CLASSES=true\r\n\r\n    Same, hangs forever on first request.\r\n\r\n3. CONF_EAGER_LOAD=true CONF_CACHE_CLASSES=false rails serve\r\n\r\n    Works fine on initial requests. However, if you change\r\n    the source of a file on disk, that gets referenced in\r\n    one of the child threads, on _next_ request the worker\r\n    thread will hang forever.\r\n\r\n4. CONF_EAGER_LOAD=true CONF_CACHE_CLASSES=true\r\n\r\n    This is the magic configuration that makes it work, at the cost of\r\n    giving up on development-mode class reloading, you need to restart\r\n    the app to pick up any changes.\r\n\r\nAdditional irrelevant config or logic:\r\n\r\n* The setting for `config.auto_load` is irrelevant. In each of the above 4 cases,\r\nwhether `config.auto_load` was set to true, false, or unset (use default) made\r\nno difference, `config.eager_load` and `config.cache_classes` were the only\r\nsettings that seemed to matter for this behavior.\r\n\r\n* I was aware of `Rails.application.reloader` from seeing it used in a\r\n  [sidekiq change](https://github.com/mperham/sidekiq/pull/2457). Without really\r\n  understanding it, I tried wrapping my child threads like so:\r\n\r\n      Concurrent::Future.execute do\r\n        Rails.application.reloader.wrap do\r\n          SomeWorker.new.value\r\n        end\r\n      end\r\n\r\n  That made no difference either, all observed behavior was the same in\r\n  above 4 configuration combinations, with or without this `wrap`. I don't\r\n  know if there's a more useful way to use the `reloader` for this use case,\r\n  or if it's just irrelevant.\r\n\r\nIssues for Rails team:\r\n\r\n1. Is this a bug?\r\n\r\n   I suspect the answer will be \"no\" -- it is no longer supported to use\r\n   threads like this without turning off development-mode class reloading\r\n   entirely.\r\n\r\n   This is a bit annoying, as I much preferred having it _mostly_ working\r\n   with occasional exceptions that made me restart the app, instead of\r\n   needing to turn off dev-mode reloading to get it to work at all, and\r\n   _always_ having to restart the app to pick up changes.\r\n\r\n   However, it may be that's just how it is, as sad as it makes me. Nevertheless:\r\n\r\n2. Can failure mode be better?\r\n\r\n    The \"failure mode\" for doing it wrong of hanging forever/deadlocking\r\n    is pretty annoying and confusing. It can take someone quite a while\r\n    to figure out what's going on, and that it's even related to auto-loading\r\n    at all.  Especially with concurrency getting easier to use for less\r\n    experienced developers (hooray ruby-concurrent), I can see people\r\n    getting _really_ stuck here they accidentally do it 'wrong' and get a\r\n    mysterious deadlock.\r\n\r\n    Is there any way to get a better failure mode, an actual quick fail with\r\n    an exception with a useful class/message, instead of a hang forever/deadlock?\r\n\r\n    Whether this is considered a 'bug' or a 'new feature' has to do with the\r\n    intentions of whoever wrote the new autoloading stuff, I guess. I couldn't\r\n    say. \r\n\r\n3. Documentation.\r\n\r\n    All of these things need better documentation -- which has been\r\n    true pre-Rails5 too, but with changes those of us who kind of\r\n    sort of figured it out in Rails <5 could use it too.\r\n\r\n    Save others the several hours I spent debugging my deadlock, investigating\r\n    the issue, resulting in this here you are reading.\r\n\r\n    I can't submit a doc PR myself, because I really don't understand\r\n    well enough what's actually going on, or the intended behavior/configuration.\r\n\r\n    Some suggested doc needs:\r\n\r\n    * `config.eager_load`, `config.cache_classes` and `config.auto_load` have\r\n       always been poorly-doc'd, especially their interactions with each other,\r\n       and their defaults, which seem to depend on how others of them are set.\r\n       Not only poorly doc'd, but fairly high 'churn' changing from Rails version\r\n       to version.\r\n        * For my use case, I only avoided deadlock with `eager_load` and\r\n          `cache_classes` both true. Is there any reason at all to have\r\n          one true and the other false? In what circumstances might\r\n          this make sense, and what does it do?\r\n\r\n        * Does `config.auto_load` do anything at all in Rails5, or has it\r\n          become a no-op?  It didn't seem to have an effect on my problem\r\n          case, but maybe it does in other cases? Or is it gone?\r\n\r\n        * If it's true that you need to turn off dev-mode reloading in\r\n          order to use threads inside a Rails action method (inside the request loop),\r\n          and it's true that you do that with `eager_loading` and `cache_classes`\r\n          both set to true, then some documentation to that effect would be\r\n          welcome, and would save people some confusing debugging time.\r\n          If i havne't actually figured out the right/best/only way to do this,\r\n          and there are other options -- I'm prob not the only one who would\r\n          appreciate some docs!\r\n\r\n       * Is there anyway to turn off _class_ reloading, to be concurrency-safe\r\n         for the auto-loader, but still have _view template_ (erb) reloading? I remember\r\n         there was in an old Rails version (3?), but the config seems to have\r\n         disappeared. \r\n\r\n        * _test_ enviornments. Default generated Rails5 test env\r\n          is `cache_classes==true`, `eager_load==false`. Rails 4\r\n          had some generated comments in `config/test.rb`\r\n          about `eager_load`: \" If you are using a tool that\r\n          preloads Rails for running tests, you may have to set it to true.\"\r\n\r\n          A bit confusing and in my experience not entirely reliable advice in Rails 4.\r\n          I believe a \"tool that preloads rails for running tests\" basically\r\n          means \"Capybara\".  Rails5 no longer generates this hint, but still defaults\r\n          `eager_load` to false. Is this going to cause problems with capybara?\r\n\r\n          Concurrency problems with capybara are super frustrating to debug\r\n          and figure out how to deal with them, so some instructions\r\n          here would be much appreciated. Especially since the default setting\r\n          is one that under my use case above still caused deadlocks.\r\n\r\n    * Rails.application.reloader\r\n\r\n      The only reason I knew this even existed, or anything at all about how to use\r\n      it, is from the [sidekiq pr](https://github.com/mperham/sidekiq/pull/2457).\r\n\r\n      The only reason it's in a sidekiq PR is because someone from Rails core\r\n      team gave sidekiq hints/code, it would be unlikely for anyone who doesn't\r\n      already know the code to have known that it should be used, and the correct\r\n      way to use it.\r\n\r\n      It may be that `Rails.application.reloader` is entirely irrelevant to\r\n      my use case -- some docs explaining what it is and how it use it could\r\n      have saved me some time in figuring that out myself by experimenting.\r\n\r\n      But clearly there are _some_ appropriate use cases for it in non-Rails-core code,\r\n      like in sidekiq. some docs are needed so people not on Rails core team know when to use it,\r\n      and how.\r\n\r\n      Tagging @matthewd , because he suggested in the sidekiq PR that I open\r\n      a Rails issue on documentation of the \"executor/reloader API\", and I'm\r\n      hoping he has some input on the general reloading/concurrency issues\r\n      above, since they look pretty intimately related to the executor/reloader\r\n      in Rails5.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/rails/rails/issues/comments/255496037","html_url":"https://github.com/rails/rails/issues/26847#issuecomment-255496037","issue_url":"https://api.github.com/repos/rails/rails/issues/26847","id":255496037,"user":{"login":"jrochkind","id":149304,"avatar_url":"https://avatars.githubusercontent.com/u/149304?v=3","gravatar_id":"","url":"https://api.github.com/users/jrochkind","html_url":"https://github.com/jrochkind","followers_url":"https://api.github.com/users/jrochkind/followers","following_url":"https://api.github.com/users/jrochkind/following{/other_user}","gists_url":"https://api.github.com/users/jrochkind/gists{/gist_id}","starred_url":"https://api.github.com/users/jrochkind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrochkind/subscriptions","organizations_url":"https://api.github.com/users/jrochkind/orgs","repos_url":"https://api.github.com/users/jrochkind/repos","events_url":"https://api.github.com/users/jrochkind/events{/privacy}","received_events_url":"https://api.github.com/users/jrochkind/received_events","type":"User","site_admin":false},"created_at":"2016-10-22T00:44:09Z","updated_at":"2016-10-22T00:44:09Z","body":"But crap! First load works fine even in dev-mode settings with recommended use of `ActiveSupport::Dependencies.interlock.permit_concurrent_loads` on blocking wait for futures, and `Rails.application.reloader.wrap do` inside future body...\r\n\r\nBut if I change the autoloaded source file on disk, and request again... it seems to still deadlock. Darnit. Okay, enough of this for the night, I'll wait for some feedback. "}}
{"actor":{"display_login":"jrochkind"}}