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

1.0 cache always misses on 1.8.6, but stores new entry, leading to very bad memory leak #45

Closed
kyledrake opened this issue Aug 4, 2010 · 3 comments

Comments

@kyledrake
Copy link

I had a better description of this, but it was on the old Lighthouse which is now closed.

Sinatra 1.0 leaks memory when the cache is enabled on 1.8.6 due to the hash within array bug, eventually eating all the memory on the system.

Example:

irb(main):006:0> example = { [{:key => 'val'}] => 'ok' }
=> {[{:key=>"val"}]=>"ok"}
irb(main):007:0> example[{:key => 'val'}]
=> nil

This works on 1.8.7 and doesn't on 1.8.6. Sinatra essentially does this for caching, so it just keeps missing and re-storing, and the hash builds up with duplicate entries.

Here's a reworked Tile::Cache#fetch that makes the test pass. It's not pretty, might be a bit verbose, but it seems to work.

def fetch(*key)
 cached_template = nil
 if RUBY_VERSION == '1.8.6'
   @cache.keys.each do |k|
     if k == key
       @cache[k] ||= yield
       cached_template = @cache[k]
     end
   end
   if cached_template.nil?
     @cache[key] = yield
     cached_template = @cache[key]
   end
 else
   @cache[key] ||= yield
   cached_template = @cache[key]
 end
 cached_template
end

Finally, here is a test for reproduction: http://github.com/kyledrake/tilt/commit/76672520040c4e4501f85a83194d7dcc562ff2dd

@rkh
Copy link
Member

rkh commented Aug 20, 2010

I think 1.8.6 is not a priority any longer, with Rack having dropped support (and Rails/Rubygems going to). Also, this is a well known Ruby bug, not a Sinatra bug.

I propose a "wont fix".

@rkh
Copy link
Member

rkh commented Sep 7, 2010

Memory leak fixed by #56.

@rkh
Copy link
Member

rkh commented Sep 25, 2010

Fix has been merged.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants