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

Memory Leak occured when use builder/nokogiri template. #1714

Closed
gassyfeve opened this issue Aug 17, 2021 · 4 comments · Fixed by #1719
Closed

Memory Leak occured when use builder/nokogiri template. #1714

gassyfeve opened this issue Aug 17, 2021 · 4 comments · Fixed by #1719

Comments

@gassyfeve
Copy link
Contributor

gassyfeve commented Aug 17, 2021

I found a memory leak issue when use inline builder/nokogiri template.

require 'sinatra'

class MyApp < Sinatra::Base
  post '/' do
    builder do |xml|
      xml.TX do
      end
    end
  end
end

template_cache grows 1 after each request in test and production environment.

@jkowens
Copy link
Member

jkowens commented Aug 30, 2021

I'm almost certain this is the culprit:

template = Proc.new { block } if template.nil?

See: https://github.com/rtomayko/tilt/blob/e7432c54338f54c9e432282a83a6b6b3deb10f77/lib/tilt.rb#L88

@gassyfeve
Copy link
Contributor Author

gassyfeve commented Aug 30, 2021

To fix this issue temporarily, I modify function compile_template in sinatra/lib/sinatra/base.rb.
I avoid to use tempalte_cache if arg data is Symbol. That fix the problem.
I think template_cache is useful for Symbol and String for avoid to load the template from disk again and again.
But it is useless for proc for they are already code.

  • Origial
    def compile_template(engine, data, options, views)
      eat_errors = options.delete :eat_errors
      template_cache.fetch engine, data, options, views do
        template = Tilt[engine]
        raise "Template engine not found: #{engine}" if template.nil?

        case data
        when Symbol
          body, path, line = settings.templates[data]
          if body
            body = body.call if body.respond_to?(:call)
            template.new(path, line.to_i, options) { body }
          else
            found = false
            @preferred_extension = engine.to_s
            find_template(views, data, template) do |file|
              path ||= file # keep the initial path rather than the last one
              if found = File.exist?(file)
                path = file
                break
              end
            end
            throw :layout_missing if eat_errors and not found
            template.new(path, 1, options)
          end
        when Proc, String
          body = data.is_a?(String) ? Proc.new { data } : data
          caller = settings.caller_locations.first
          path = options[:path] || caller[0]
          line = options[:line] || caller[1]
          template.new(path, line.to_i, options, &body)
        else
          raise ArgumentError, "Sorry, don't know how to render #{data.inspect}."
        end
      end
    end
  end
  • Fixed
    def compile_template(engine, data, options, views)
      eat_errors = options.delete :eat_errors
      template = Tilt[engine]
      raise "Template engine not found: #{engine}" if template.nil?

      case data
      when Symbol
        template_cache.fetch engine, data, options, views do
          body, path, line = settings.templates[data]
          if body
            body = body.call if body.respond_to?(:call)
            template.new(path, line.to_i, options) { body }
          else
            found = false
            @preferred_extension = engine.to_s
            find_template(views, data, template) do |file|
              path ||= file # keep the initial path rather than the last one
              if found = File.exist?(file)
                path = file
                break
              end
            end
          end
          throw :layout_missing if eat_errors and not found
          template.new(path, 1, options)
        end
      when Proc
        caller = settings.caller_locations.first
        path = options[:path] || caller[0]
        line = options[:line] || caller[1]
        template.new(path, line.to_i, options, &data)
      when String
        template_cache.fetch engine, data, options, views do
          body = Proc.new { data } 
          caller = settings.caller_locations.first
          path = options[:path] || caller[0]
          line = options[:line] || caller[1]
          template.new(path, line.to_i, options, &body)
        end
      else
        raise ArgumentError, "Sorry, don't know how to render #{data.inspect}."
      end
    end
  end

@jkowens
Copy link
Member

jkowens commented Aug 30, 2021

Looks good, are you able to create a PR? One thought could we extract the following to a new method?

caller = settings.caller_locations.first
path = options[:path] || caller[0]
line = options[:line] || caller[1]
template.new(path, line.to_i, options, &body)

@gassyfeve
Copy link
Contributor Author

Sure. Let met check UT first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants