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 leakage since 2.4.0 #460

Open
lizdeika opened this issue Dec 22, 2020 · 11 comments
Open

Memory leakage since 2.4.0 #460

lizdeika opened this issue Dec 22, 2020 · 11 comments

Comments

@lizdeika
Copy link

lizdeika commented Dec 22, 2020

We have a couple of applications where postmark uses this gem.
After updating bunch of gems we noticed a memory leak in sidekiq processes(we have a script that restarts sidekiq when memory is bloated).
We had to revert all the recent gem updates and the problem was gone.
Then we updated gems back one by one to find out which one caused the leakage.
The problem returned back with json gem 2.4.0 update and reverting back to 2.3.1 was all good once again.

@nitaliano
Copy link

nitaliano commented Mar 22, 2021

Seeing the same exact issue in the project i'm working on recently updated from 2.3.0 -> 2.5.1 and server memory is growing overtime

@marcandre
Copy link
Contributor

@lizdeika and @nitaliano that's pretty serious, thanks for pinpointing to json and a precise version too.

If I'm not mistaken, there were really only two changes between 2.3.1 and 2.4: #405 and #447. I reread the diff and nothing jumps at me.

It would help to know:

  1. Which Ruby version are you using?
  2. Are you parsing JSON, generating JSON, or both?
  3. Are you using, directly or via a dependency, the new freeze: true option when parsing, or the escape_slash: true when generating?

Question 3 could be partially answered by checking the options passed to JSON::Parser.new like this:

require 'json'
require 'set'
$json_options = Set.new

module OptionPeeker
  def initialize(source, **opts)
    if $json_options.add?(opts)
      p "New JSON parser options:", opts
    end
    super
  end
end

JSON::Parser.prepend OptionPeeker

(You may want to replace p with some other output mechanism)

Of course, a script we could run to reproduce the leak would be the best...

@marcandre
Copy link
Contributor

Also, if either of you is using jruby, or the pure-ruby version of json, make sure to say it...

@lizdeika
Copy link
Author

it was CRuby 2.6.5 compiled against jemalloc 5.2.1
sorry, no idea how postmark gem uses it
unfortunately can not run the script as I have no access to the project already

@nitaliano
Copy link

nitaliano commented Mar 24, 2021

Here are some extra details

  1. Ruby version 2.6.6
  2. Use Case: parsing and generating; I scanned through our code its its mostly using JSON.parse, JSON.dump, and JSON.pretty_generate. It looks like it is mostly being used for parsing HTTP request bodies, and generating JSON for logs which I am the most suspicious about.
  3. We are using it directly as a dep

We are also using sidekiq like @lizdeika. Hopefully next time I post back I'll have a repo to reproduce it 😄

@marcandre
Copy link
Contributor

To better pinpoint the problem, I created 2 branches of json, starting from 2.3.1, by adding the feature #447 (branch "freeze_231") and then also adding #405, which I believe is equivalent to 2.4.1 for the C code (branch "equiv_241")

So if it was possible to check using:

gem 'json', git: 'https://github.com/marcandre/json.git', tag: 'freeze_231'
# and if that does not leak, then try with:
gem 'json', git: 'https://github.com/marcandre/json.git', tag: 'equiv_241'

@luke-gru
Copy link

luke-gru commented Jan 30, 2023

I know this is an old issue, but I'm interested in it nonetheless because I noticed something in this gem that could cause issues.

Are the JSON keys you're parsing known ahead of time and bounded to a certain set of strings, or are they unbounded?

If they're unbounded, this commit: 1982070cb84a38793277f6359387938d80e4d2c4 introduces a memory issue by keeping all json keys in the ruby VM's frozen string table (where they're never released, as far as I know).

cc @byroot

@byroot
Copy link
Contributor

byroot commented Jan 30, 2023

(where they're never released, as far as I know).

That's incorrect. "fstrings" (AKA interned strings) are garbage collectable.

@luke-gru
Copy link

Oh okay right, I just took at look at rb_str_free(), carry on then 😄

@luke-gru
Copy link

This might be nothing too, but in the parser there's a call to ALLOC_N and it gets free'd with free instead of xfree, could that be a source of memory leak if ruby is built a certain way? I've heard that it's an antipattern but I don't know why.

@byroot
Copy link
Contributor

byroot commented Jan 31, 2023

I've heard that it's an antipattern but I don't know why.

AFAIK, it's mostly because that skip letting the GC know that some memory was freed, so GC might trigger earlier than it should but that's about it.

Would be nice to fix it though.

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

5 participants