Use "main" reloader and cache resolvers from {append,prepend}_view_path #47347
Conversation
| @watcher.execute | ||
| else | ||
| @watcher.execute_if_updated | ||
| @previous_change ||= old_watcher&.updated? |
There was a problem hiding this comment.
Not really practical to cover the race in a test, so maybe worth a comment here noting that we specifically do the final query of the old watcher after the new one has been set up?
|
I've tested this branch and confirm it fixes the described issue of running Rails on Falcon. Fixes socketry/falcon#166. |
|
Awesome, thanks. I would not have been able to come up with such a profound solution 💪 |
|
@jhawthorn can we merge this? |
When prepending or appending view paths, Rails will create an ActionView::Resolver for each String or Pathname argument. Previously, these were not cached. If view paths were added in a request context (which is a common thing in a multi-tenant application), new resolvers were created on each request. This effectively disabled template caching for the added view paths. Further, under certain circumstances, it also created a memory leak. This commit fixes both issues by turning dynamic view path strings into cached resolvers. Co-authored-by: Dominik Schöler <dominik.schoeler@makandra.de> Co-authored-by: John Hawthorn <john@hawthorn.email>
Co-authored-by: Matthew Draper <matthew@trebex.net>
jhawthorn
force-pushed
the
view_cache_main_reloader
branch
from
abf52a6
to
72abd63
Compare
|
Thanks this is amazing 🤩 |
|
@jhawthorn did you happen to record any relative metrics for before and after this PR? I've seen some microbenchmarks in an older issue that pointed to this as the root cause. But I'm also curious if it'll affect real world performance. |
|
Based on prior discussions, isn't this only used in development? |
|
As noted in #46345, the missing caching of dynamic invocations of |
|
@ioquatix it is used to render different layouts/views based on context set at runtime. For example one client should use theme1 while other theme2 (different paths). |
This is a refactor of the `Registry` module added in rails#47347. This is an attempt to minimize the namespace conflcits that will happen when users will have a top level `Registry` module which can cause incorrect behavior Replace ActionView::ViewPaths::Registry with ActionView::PathRegistry
This fixes issues with runtime use of
{append,prepend}_view_pathand also rewritesActionView::CacheExpiryto hook into the main reloaders rather than holding its own lock.Supersedes #46345 and #47257
A potential downside is that this may cause increased reloading of classes in a dev environment, as previously view reloading was separate. I think that's acceptable as other components work that way (like i18n), if we want to improve it we can investigate in the future.
In order to track directories to watch for view changes, we added a callback around building of
FileSystemResolvers, to start watching the directories of any newly created resolvers immediately after they're created. This should avoid us getting stuck in a reload loop where loading a controller class creates a new resolver which causes classes to be reloaded.This should also fix issues with
{append,prepend}_view_pathwhen used at runtime (not at the class level). Previously the resolvers did not persist between requests and a new one was created each time. In order to do this we now keep any resolvers created around for the entire lifetime of the application, including (at least currently) in development.cc @codener