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

Possible memory leak in prepend_view_path #14301

Closed
r38y opened this issue Mar 6, 2014 · 19 comments
Closed

Possible memory leak in prepend_view_path #14301

r38y opened this issue Mar 6, 2014 · 19 comments

Comments

@r38y
Copy link

@r38y r38y commented Mar 6, 2014

I think I'm seeing a memory leak when using prepend_view_path.

When I use the instance method in a controller:

def setup_view_paths
  prepend_view_path(magazine_views)
end

I get a lot of leaked strings (and other objects in the same area)

Leaked 1384 STRING objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:299
Leaked 692 DATA objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:299
Leaked 616 ARRAY objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:299
Leaked 396 ARRAY objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:300
Leaked 395 DATA objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:106
Leaked 235 STRING objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:332
Leaked 197 NODE objects at: /Users/r38y/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/actionpack-4.0.2/lib/action_view/template.rb:299

That look like _app_views_site_overrides_j____com_posts_shared__featured_html_erb___3233500999093705389_70234272758740.

When I use the class method in an initializer:

ActionController::Base.prepend_view_path('app/views/site_overrides/j-14.com')

I don't leak anything in template.rb.

I need to use the instance method because I need to set things up for themes.

I've seen this in production and development.

This is how I'm getting that information: https://gist.github.com/r38y/02f4378a19f9064d2da2

I've been in the Rails source code for a couple days and cannot figure out why it is leaking 😦. Any ideas would would a ton. Thank you!

@r38y
Copy link
Author

@r38y r38y commented Mar 6, 2014

If I render the partial like:

<%= render 'site_overrides/j-14.com/posts/shared/featured', post: post %>

I don't see the issue.

@r38y
Copy link
Author

@r38y r38y commented Mar 6, 2014

It looks like it is the string for the method name that is leaking

If I change

# Make sure that the resulting String to be evalled is in the
# encoding of the code
source = <<-end_src
  def #{method_name}(local_assigns, output_buffer)
    _old_virtual_path, @virtual_path = @virtual_path, #{@virtual_path.inspect};_old_output_buffer = @output_buffer;#{locals_code};#{code}
  ensure
    @virtual_path, @output_buffer = _old_virtual_path, _old_output_buffer
  end
end_src

to

source = ''
source << "def #{method_name}(local_assigns, output_buffer)\n"
source << "_old_virtual_path, @virtual_path = @virtual_path, #{@virtual_path.inspect};_old_output_buffer = @output_buffer;#{locals_code};#{code}\n"
source << "ensure\n"
source << "@virtual_path, @output_buffer = _old_virtual_path, _old_output_buffer\n"
source << 'end\n'

I no longer the partials memory leaks. I seem to only get the main template method name leak:

_app_views_site_overrides_j____com_pages_index_html_erb___116067111201229435_70221803182180

@r38y
Copy link
Author

@r38y r38y commented Mar 6, 2014

Changing

source = <<-end_src
  def #{method_name}(local_assigns, output_buffer)
    _old_virtual_path, @virtual_path = @virtual_path, #{@virtual_path.inspect};_old_output_buffer = @output_buffer;#{locals_code};#{code}
  ensure
    @virtual_path, @output_buffer = _old_virtual_path, _old_output_buffer
  end
end_src

to

source.freeze = <<-end_src
  def #{method_name}(local_assigns, output_buffer)
    _old_virtual_path, @virtual_path = @virtual_path, #{@virtual_path.inspect};_old_output_buffer = @output_buffer;#{locals_code};#{code}
  ensure
    @virtual_path, @output_buffer = _old_virtual_path, _old_output_buffer
  end
end_src

Seems to fix my template leak problem

@r38y
Copy link
Author

@r38y r38y commented Mar 6, 2014

Turns out, #freeze didn't work. I didn't have a sanity check that the page was actually rendering.

@r38y
Copy link
Author

@r38y r38y commented Mar 6, 2014

Even thought he #freeze was a wrong fix, there is still a difference if I use the class method vs the instance method

I dumped the view_paths after each of three requests using both techniques to see if there were any differences:

Using the class method in an initializer vs instance method in a before filter: https://gist.github.com/r38y/9400923

The difference I noticed is using the class method resulted in exactly the same instances for each of the objects but the instance method version resulted in different instance methods for

ActionView::OptimizedFileSystemResolver
ActionView::Resolver::Cache
ActionView::Resolver::Cache::SmallCache

I think it makes sense (maybe) but I don't think the strings associated with them are being released.

@JustMikey
Copy link

@JustMikey JustMikey commented Mar 15, 2014

I've just noticed the same issue in my app with prepend_view_path and you saved me a lot of time by saying that it can be put it in an initializer, I didnt even think of it somehow :) Hope they'll fix it

@r38y
Copy link
Author

@r38y r38y commented Mar 15, 2014

I'm glad it helped a bit!

I have quite a few overrides depending on the request so the way I got around it was to create a registry in the initializer:

PATH_SET_REGISTRY = {}
Dir.entries('app/views/site_overrides').reject{|e| e.in?(['.', '..'])}.each do|domain|
  paths = ["app/views/site_overrides/#{domain}"]
  PATH_SET_REGISTRY[domain] = ActionView::PathSet.new(paths)
end
PATH_SET_REGISTRY

And then look up the override paths in that registry on each request:

def setup_view_paths
  prepend_view_path PATH_SET_REGISTRY[current_magazine.domain]
end

I still think there is a memory leak in Rails because I think we should be using the same path and cache instances for each lookup path between requests.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 28, 2014

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Mar 28, 2014

Can you prove the objects are actually being leaked by putting this code in an infinite loop and showing unbounded memory growth?

Also a sample app would really help.

@r38y
Copy link
Author

@r38y r38y commented Mar 28, 2014

I created a fresh, small app that has one action with prepend_view_path and one that doesn't:

https://github.com/r38y/prepend_view_path_example

/prepended has prepend_view_path and /not_prepended does not.

I used apache bench to hit it a bunch of times and it seemed like the one grew more than the other one, BUT, it is hard to see when the templates are so small. Our app is ~huge.

Maybe someone can help me figure out how to get more info from it?

@tenderlove
Copy link
Member

@tenderlove tenderlove commented May 7, 2014

What version of Ruby are you using? Also, does it require that you use apache, or do you see the same leak on different web servers? FWIW, I'm running with trunk Ruby and using WEBrick in production mode. I see large swings in memory usage, but it isn't growing unbounded.

@rails-bot
Copy link

@rails-bot rails-bot commented Aug 19, 2014

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot rails-bot closed this Nov 19, 2014
@rails-bot
Copy link

@rails-bot rails-bot commented Nov 19, 2014

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@belov
Copy link

@belov belov commented Jul 21, 2015

I confirm this issue.
This https://github.com/belov/view_template_leak_app contain results some tests and increase memory chart.
Please see.

testable env:

  • ubuntu 14.04
  • ruby 2.2.2
  • rails 4.2.3
  • unicorn 4.9.0

@scottwillson
Copy link
Contributor

@scottwillson scottwillson commented Dec 14, 2020

Still happens with Ruby 2.7 and Rails 6.0 (possibly worse in Rails 6 vs. 5). Per-request prepend_view_path causes memory use to slowly increase without bound. Removing that call made a dramatic difference in my app's memory usage. It's an old app (began on Rails 2) with some complicated view building, so perhaps there's an interaction with something else I'm doing.
memory

@espen
Copy link

@espen espen commented Dec 14, 2020

The problem is that ActionView::PathSet is generated for each request and results in a memory leak. If you cache the response from ActionView::OptimizedFileSystemResolver then there is no problem using prepend_view_path. The Rails Guides currently recommends setting prepend_view_path pr request with a string that leads to this problem. I had a major memory leak in my app and setting a cached ActionView::OptimizedFileSystemResolver into prepend_view_path solved the issue.

See also https://www.spacevatican.org/2019/5/4/debugging-a-memory-leak-in-a-rails-app/

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 17, 2020

@espen good point. Can you open a PR to change the guide to set the view path to a cached Resolver instance instead of a string?

@mudge
Copy link
Contributor

@mudge mudge commented Feb 2, 2021

While using a persistent Resolver instance also fixed a memory leak for me, I would caution that it is causing issues when Action View caching is disabled (e.g. in development mode).

Whenever I update a template, I get an undefined method error with a cached template name (e.g. _app_views...) due to the cache inside the persistent Resolver not being cleared by Action View Cache Expiry which relies on ActionView::ViewPaths.all_view_paths which doesn't contain view paths prepended only to the current request's Lookup Context.

I'm not sure what the right thing to do in this case is other than only using a persistent Resolver when Action View caching is enabled (falling back to using a String or Pathname with prepend_view_path so that a new Resolver is created for every request but re-introduces the memory leak). Alternatively, one could forcibly call clear_cache on any persistent resolver as long as ActionView::Resolver.caching? is false, e.g.

SUBDOMAIN_SPECIFIC_TEMPLATES = {
  'subdomain1' => ActionView::OptimizedFileSystemResolver.new('app/views/subdomain1'),
  'subdomain2' => ActionView::OptimizedFileSystemResolver.new('app/views/subdomain2'),
  'subdomain3' => ActionView::OptimizedFileSystemResolver.new('app/views/subdomain3')
}.freeze

def prepend_subdomain_specific_templates
  subdomain_specific_templates = SUBDOMAIN_SPECIFIC_TEMPLATES.fetch(request.subdomain)
  subdomain_specific_templates.clear_cache unless ActionView::Resolver.caching?

  prepend_view_path(subdomain_specific_templates)
end

@woodhull
Copy link

@woodhull woodhull commented Feb 11, 2021

Having the exact same issue as @mudge after upgrading a rails app to v6. We have a Concurrent::Map of ActionView::OptimizedFileSystemResolver that we prepend per-request.

I looked into trying to figure out how to hook into the cache clear events to clear those too in addition to the resolvers that rails creates, but couldn't figure out how to wire that up. Clearing on every request in development mode works though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet