intridea / multi_json

A generic swappable back-end for JSON handling.
http://rdoc.info/projects/intridea/multi_json
MIT License
757 stars 129 forks source link

Memory Leak when upgrading from 1.11.3 to 1.12.0 #168

Closed shaw3257 closed 8 years ago

shaw3257 commented 8 years ago

Hello, We have a Rails App on Heroku and we noticed a memory leak after upgrading from 1.11.3 to 1.12.0 image

We traced it down to the following line of code https://github.com/intridea/multi_json/commit/7aaef2a1bc2b83c95e4208b12dad5d1d87ff20a6#diff-e01bdf5b87e98ba8fb217099bc4b328fR12

You can easily replicate this by having key contain a proc -- which is causing cache misses that endlessly grow the object.

He's an example of the keys within the cache object after two identical requests:

{ 
  :prefixes=>["api/v1/foo", "application"], 
  :template=>"index", 
  :layout=>#<Proc:0x007f8188b55e08@/gems/actionpack-3.2.22/lib/abstract_controller/layouts.rb:383>
}

{ 
  :prefixes=>["api/v1/foo", "application"], 
  :template=>"index", 
  :layout=>#<Proc:0x007f8197152b40@/gems/actionpack-3.2.22/lib/abstract_controller/layouts.rb:383>
}
rwz commented 8 years ago

Damn, this sucks. I guess we'll have to come up with cache eviction policy of some sort. Thanks for reporting this. I recommend downgrading back to 1.11.3 until this gets fixed.

Could you also try tracing, where these options are coming from? I wonder if it's coming from your own code, or from some rails internals. Seems like the performance could be improved significantly if it wasn't generating 100% cache misses.

shaw3257 commented 8 years ago

Good news. I don't think it's a widespread issue with Rails. I traced it back to this rails-patch-json-encode gem that we use: https://github.com/GoodLife/rails-patch-json-encode/blob/master/lib/rails/patch/json/encode.rb#L29. Which is forwarding proc options from Rails: https://github.com/rails/rails/blob/52ce6ece8c8f74064bb64e0a0b1ddd83092718e1/actionview/lib/action_view/layouts.rb#L386

In any case, I like your idea of cache eviction for multi-json -- It should handle cases where options contain non-scalar types.

rwz commented 8 years ago

Oh my, this rails-patch-json-encode thing should probably stop sending rails options to MultiJson. None of them are compatible and at this point it only generates memory leaks and cache misses.

rwz commented 8 years ago

Fixed and released 1.12.1

shaw3257 commented 8 years ago

Thanks @rwz !

onyxraven commented 8 years ago

fwiw, we saw a very large memory leak in production upgrading to 1.12.1 - we rolled back to the 1.11.x series and the leak stopped.

I haven't yet had time to dig into the cause though.

rwz commented 8 years ago

@onyxraven huh? That's interesting. I run several systems on 1.12.1 and haven't seen any leaks so far. Please let me know if you able to find out something.