Fix memory leak with ruby/require/autoload_paths #16837

NickLaMuro · 2018-01-17T21:48:54Z

In ruby versions less than 2.5.0, it was found that if you have Pathname objects in the $LOAD_PATH instead of regular strings, it messes with ruby's internals and causes a memory leak.

In most ruby applications, this wouldn't be much of an issue, but there are a significant number of deferred require statements that we do in manageiq that cause this to leak overtime. Further more, this seems to be an issue that presents itself even if the file has been loaded previously (mostly a no-op for require).

Replication of this issue can be done using a simple ruby script:

require 'pathname'
require 'fileutils'

puts Process.pid

Dir.mkdir("foo") unless Dir.exists?("foo")
$LOAD_PATH.unshift(Pathname.new("foo"))

FileUtils.touch("empty.rb")
1500.times { 1500.times { require "empty" }; print "."; GC.start; }

By simply running the application with the Pathnames converted to strings, this should be a proper and low cost workaround for us until it is patched in ruby or we update manageiq to >= ruby 2.5.0.

This will also need to be done in the provider repos as well that happen to add to the autoload_paths in their engines as well. Should only be the manageiq-api, manageiq-ui-classic, and the manageiq-automation_engine (PRs for those to come shortly, and will be linked below)

Links

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1535720

TODO: Add links for the following

Additional PRs for the provider repos
Link to ruby bug
Relavent BZs once we determine which workers this helps fix completely

Steps for Testing/QA

Running the script above should replicate the issue on any system.

Additionally, we are testing this collection of patches on multiple appliances to confirm that the leak is removed.

Finally, once all these are merged (or by applying these patches locally), you can run a bin/rails console and confirm that nothing in the $LOAD_PATH is a Pathname:

irb> $LOAD_PATH.select {|path| path.class == Pathname }
#=> []

In ruby versions less than 2.5.0, it was found that if you have Pathname objects in the $LOAD_PATH instead of regular strings, it messes with ruby's internals and causes a memory leak. In most ruby applications, this wouldn't be much of an issue, but there are a significant number of deferred require statements that we do in manageiq that cause this to leak overtime. Further more, this seems to be an issue that presents itself even if the file has been loaded previously (mostly a no-op for require). Replication of this issue can be done using a simple ruby script: require 'pathname' require 'fileutils' puts Process.pid Dir.mkdir("foo") unless Dir.exists?("foo") $LOAD_PATH.unshift(Pathname.new("foo")) FileUtils.touch("empty.rb") 1500.times { 1500.times { require "empty" }; print "."; GC.start; } By simply running the application with the Pathnames converted to strings, this should be a proper and low cost workaround for us until it is patched in ruby or we update manageiq to >= ruby 2.5.0.

NickLaMuro · 2018-01-17T21:52:05Z

@miq-bot add_label bug, performance

miq-bot · 2018-01-17T22:08:09Z

Checked commit NickLaMuro@4fb6f0b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

kbrock

good find

jrafanie · 2018-01-17T22:32:59Z

Awesome sauce @NickLaMuro.

jrafanie · 2018-01-17T22:33:55Z

config/application.rb

+    config.autoload_paths << Rails.root.join("lib").to_s
+    config.autoload_paths << Rails.root.join("lib", "services").to_s
+
+    config.autoload_once_paths << Rails.root.join("lib", "vmdb", "console_methods.rb").to_s


we probably want to link to the ruby bug here so we can fix this hack when ruby 2.3/2.4 are fixed

Agreed. Good call.

Going to merge anyway...the comment can be added when we add the spec we discussed in Gitter.

chrisarcand · 2018-01-18T03:17:39Z

Edited your description to add manageiq-graphql 💕

NickLaMuro · 2018-01-18T03:51:13Z

@chrisarcand Thanks, you're the best!

NickLaMuro · 2018-01-18T21:58:19Z

@miq-bot add_labels euwe/yes, fine/yes, gaprindashvili/yes

Fix memory leak with ruby/require/autoload_paths (cherry picked from commit 193d031) https://bugzilla.redhat.com/show_bug.cgi?id=1536658

simaishi · 2018-01-19T20:51:06Z

Gaprindashvili backport details:

$ git log -1
commit 0de153c6539a54b519675b8ce6e5168d43e7a874
Author: Jason Frey <[email protected]>
Date:   Thu Jan 18 12:32:34 2018 -0500

    Merge pull request #16837 from NickLaMuro/leak_fix_to_s_autoload_paths
    
    Fix memory leak with ruby/require/autoload_paths
    (cherry picked from commit 193d0315ac971c43214cbc4795017959e1031ad4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536658

Fix memory leak with ruby/require/autoload_paths (cherry picked from commit 193d031) https://bugzilla.redhat.com/show_bug.cgi?id=1536672

simaishi · 2018-01-19T21:32:23Z

Fine backport details:

$ git log -1
commit ccc45b26e907a3f3f10a3617d046798fd1715cc8
Author: Jason Frey <[email protected]>
Date:   Thu Jan 18 12:32:34 2018 -0500

    Merge pull request #16837 from NickLaMuro/leak_fix_to_s_autoload_paths
    
    Fix memory leak with ruby/require/autoload_paths
    (cherry picked from commit 193d0315ac971c43214cbc4795017959e1031ad4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536672

Fix memory leak with ruby/require/autoload_paths (cherry picked from commit 193d031) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536692

simaishi · 2018-01-19T23:45:32Z

Euwe backport details:

$ git log -1
commit d3da5a5d98c0631673575916b969a6e65719db27
Author: Jason Frey <[email protected]>
Date:   Thu Jan 18 12:32:34 2018 -0500

    Merge pull request #16837 from NickLaMuro/leak_fix_to_s_autoload_paths
    
    Fix memory leak with ruby/require/autoload_paths
    (cherry picked from commit 193d0315ac971c43214cbc4795017959e1031ad4)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536692

…ents Add comments and specs around the ruby 'require leak patch' (see #16837)

…ents Add comments and specs around the ruby 'require leak patch' (see #16837) (cherry picked from commit 9bbd984)

…oad_paths Fix memory leak with ruby/require/autoload_paths (cherry picked from commit 193d031) https://bugzilla.redhat.com/show_bug.cgi?id=1536672

…and_comments Add comments and specs around the ruby 'require leak patch' (see ManageIQ#16837) (cherry picked from commit 9bbd984)

miq-bot added bug performance labels Jan 17, 2018

kbrock approved these changes Jan 17, 2018

View reviewed changes

gtanzillo approved these changes Jan 17, 2018

View reviewed changes

jrafanie reviewed Jan 17, 2018

View reviewed changes

NickLaMuro mentioned this pull request Jan 17, 2018

[WIP] Load linux_admin once when needed ManageIQ/manageiq-gems-pending#327

Closed

chrisarcand mentioned this pull request Jan 18, 2018

Fix memory leak with Pathnames on LOAD_PATH ManageIQ/manageiq-graphql#34

Merged

Fryguy merged commit 193d031 into ManageIQ:master Jan 18, 2018

Fryguy added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018

NickLaMuro mentioned this pull request Jan 18, 2018

Add comments and specs around the ruby 'require leak patch' (see #16837) #16847

Merged

miq-bot added euwe/yes labels Jan 18, 2018

Fryguy added the blocker label Jan 19, 2018

simaishi pushed a commit that referenced this pull request Jan 19, 2018

Merge pull request #16837 from NickLaMuro/leak_fix_to_s_autoload_paths

0de153c

Fix memory leak with ruby/require/autoload_paths (cherry picked from commit 193d031) https://bugzilla.redhat.com/show_bug.cgi?id=1536658

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 19, 2018

simaishi pushed a commit that referenced this pull request Jan 19, 2018

Merge pull request #16837 from NickLaMuro/leak_fix_to_s_autoload_paths

ccc45b2

Fix memory leak with ruby/require/autoload_paths (cherry picked from commit 193d031) https://bugzilla.redhat.com/show_bug.cgi?id=1536672

simaishi added fine/backported and removed fine/yes labels Jan 19, 2018

simaishi added euwe/backported and removed euwe/yes labels Jan 19, 2018

jrafanie added a commit that referenced this pull request Jan 23, 2018

Merge pull request #16847 from NickLaMuro/require_leak_tests_and_comm…

9bbd984

…ents Add comments and specs around the ruby 'require leak patch' (see #16837)

simaishi pushed a commit that referenced this pull request Mar 7, 2018

Merge pull request #16847 from NickLaMuro/require_leak_tests_and_comm…

1109712

…ents Add comments and specs around the ruby 'require leak patch' (see #16837) (cherry picked from commit 9bbd984)

simaishi pushed a commit that referenced this pull request Apr 16, 2018

Merge pull request #16847 from NickLaMuro/require_leak_tests_and_comm…

2c3f778

…ents Add comments and specs around the ruby 'require leak patch' (see #16837) (cherry picked from commit 9bbd984)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak with ruby/require/autoload_paths #16837

Fix memory leak with ruby/require/autoload_paths #16837

NickLaMuro commented Jan 17, 2018 •

edited

Loading

NickLaMuro commented Jan 17, 2018

miq-bot commented Jan 17, 2018

kbrock left a comment

jrafanie commented Jan 17, 2018

jrafanie Jan 17, 2018

NickLaMuro Jan 17, 2018

Fryguy Jan 18, 2018

chrisarcand commented Jan 18, 2018

NickLaMuro commented Jan 18, 2018 •

edited by chrisarcand

Loading

NickLaMuro commented Jan 18, 2018

simaishi commented Jan 19, 2018

simaishi commented Jan 19, 2018

simaishi commented Jan 19, 2018

Fix memory leak with ruby/require/autoload_paths #16837

Fix memory leak with ruby/require/autoload_paths #16837

Conversation

NickLaMuro commented Jan 17, 2018 • edited Loading

Links

Steps for Testing/QA

NickLaMuro commented Jan 17, 2018

miq-bot commented Jan 17, 2018

kbrock left a comment

Choose a reason for hiding this comment

jrafanie commented Jan 17, 2018

jrafanie Jan 17, 2018

Choose a reason for hiding this comment

NickLaMuro Jan 17, 2018

Choose a reason for hiding this comment

Fryguy Jan 18, 2018

Choose a reason for hiding this comment

chrisarcand commented Jan 18, 2018

NickLaMuro commented Jan 18, 2018 • edited by chrisarcand Loading

NickLaMuro commented Jan 18, 2018

simaishi commented Jan 19, 2018

simaishi commented Jan 19, 2018

simaishi commented Jan 19, 2018

NickLaMuro commented Jan 17, 2018 •

edited

Loading

NickLaMuro commented Jan 18, 2018 •

edited by chrisarcand

Loading