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 · 14 comments · Fixed by #659
Open

Memory leakage since 2.4.0 #460

lizdeika opened this issue Dec 22, 2020 · 14 comments · Fixed by #659
Labels

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
Member

@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
Member

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
Member

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
Contributor

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
Member

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
Contributor

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

@luke-gru
Copy link
Contributor

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
Member

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.

@byroot byroot added the bug label Oct 18, 2024
@casperisfine
Copy link

Alright, not sure if it's the same one, but I did find a leak:

require 'json'

data = 10_000.times.to_a << BasicObject.new
20.times do
  100.times do
    begin
      data.to_json
    rescue NoMethodError
    end
  end
  puts `ps -o rss= -p #{$$}`
end
 20128
 24992
 29920
 34672
 39600
 44336
 49136
 53936
 58816
 63616
 68416
 73232
 78032
 82896
 87696
 92528
 97408
102208
107008
111808

The various #to_json leak memory if an exception is raised during generation. Shouldn't be too hard to fix.

casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 29, 2024
Fix: ruby#460

The various `to_json` methods must rescue exceptions
to free the buffer.

```
require 'json'

data = 10_000.times.to_a << BasicObject.new
20.times do
  100.times do
    begin
      data.to_json
    rescue NoMethodError
    end
  end
  puts `ps -o rss= -p #{$$}`
end
```

```
 20128
 24992
 29920
 34672
 39600
 44336
 49136
 53936
 58816
 63616
 68416
 73232
 78032
 82896
 87696
 92528
 97408
102208
107008
111808
```
@byroot byroot closed this as completed in d227d22 Oct 30, 2024
byroot added a commit to byroot/json that referenced this issue Oct 30, 2024
Fix: ruby#460

The various `to_json` methods must rescue exceptions
to free the buffer.

```
require 'json'

data = 10_000.times.to_a << BasicObject.new
20.times do
  100.times do
    begin
      data.to_json
    rescue NoMethodError
    end
  end
  puts `ps -o rss= -p #{$$}`
end
```

```
 20128
 24992
 29920
 34672
 39600
 44336
 49136
 53936
 58816
 63616
 68416
 73232
 78032
 82896
 87696
 92528
 97408
102208
107008
111808
```
@byroot
Copy link
Member

byroot commented Oct 30, 2024

I checked the leak I just fixed predates 2.4.0, so I'll reopen, but without more info this issue isn't really actionable.

I think the only action I could see would be to setup ruby_memcheck and see if it catches something.

@byroot byroot reopened this Oct 30, 2024
casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 30, 2024
Hoping it might find the leak reported in ruby#460
casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 30, 2024
Hoping it might find the leak reported in ruby#460
casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 30, 2024
Hoping it might find the leak reported in ruby#460
casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 30, 2024
Hoping it might find the leak reported in ruby#460
casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 30, 2024
Hoping it might find the leak reported in ruby#460
casperisfine pushed a commit to casperisfine/json that referenced this issue Oct 30, 2024
Hoping it might find the leak reported in ruby#460
hsbt pushed a commit to hsbt/ruby that referenced this issue Nov 1, 2024
Fix: ruby/json#460

The various `to_json` methods must rescue exceptions
to free the buffer.

```
require 'json'

data = 10_000.times.to_a << BasicObject.new
20.times do
  100.times do
    begin
      data.to_json
    rescue NoMethodError
    end
  end
  puts `ps -o rss= -p #{$$}`
end
```

```
 20128
 24992
 29920
 34672
 39600
 44336
 49136
 53936
 58816
 63616
 68416
 73232
 78032
 82896
 87696
 92528
 97408
102208
107008
111808
```

ruby/json@d227d225ca
hsbt pushed a commit to hsbt/ruby that referenced this issue Nov 1, 2024
Hoping it might find the leak reported in ruby/json#460

ruby/json@08635312e5
hsbt pushed a commit to ruby/ruby that referenced this issue Nov 1, 2024
Hoping it might find the leak reported in ruby/json#460

ruby/json@08635312e5
@hsbt hsbt reopened this Nov 1, 2024
@luke-gru
Copy link
Contributor

luke-gru commented Nov 1, 2024

@casperisfine Funny enough I found a similar leak many years ago:

#251

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

Successfully merging a pull request may close this issue.

7 participants