From 1300a54477f0a3f04a236849f57915cdb60d77d3 Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 8 Oct 2024 14:09:31 +0200 Subject: [PATCH 1/3] add memory leaks testing like in dd-trace-rb --- .github/workflows/test-memory-leaks.yml | 15 +++++++ Gemfile | 4 +- Rakefile | 13 ++++++ spec/datadog/ci/release_gem_spec.rb | 1 + suppressions/ruby-3.4.supp | 53 +++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/test-memory-leaks.yml create mode 100644 suppressions/ruby-3.4.supp diff --git a/.github/workflows/test-memory-leaks.yml b/.github/workflows/test-memory-leaks.yml new file mode 100644 index 00000000..c5276ae7 --- /dev/null +++ b/.github/workflows/test-memory-leaks.yml @@ -0,0 +1,15 @@ +name: Test for memory leaks +on: [push] +jobs: + test-memcheck: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: 3.4.0-preview1 # TODO: Use stable version once 3.4 is out + bundler-cache: true # runs 'bundle install' and caches installed gems automatically + bundler: latest + cache-version: v1 # bump this to invalidate cache + - run: sudo apt install -y valgrind && valgrind --version + - run: bundle exec rake compile spec:ddcov_memcheck diff --git a/Gemfile b/Gemfile index e4f80a19..ed2fa82f 100644 --- a/Gemfile +++ b/Gemfile @@ -33,12 +33,14 @@ gem "pimpmychangelog", ">= 0.1.2" gem "simplecov" gem "simplecov-cobertura", "~> 2.1.0" -# type checking +# type checks, memory checks, etc. group :check do if RUBY_VERSION >= "3.0.0" && RUBY_PLATFORM != "java" gem "rbs", "~> 3.5.0", require: false gem "steep", "~> 1.7.0", require: false end + + gem "ruby_memcheck", ">= 3" if RUBY_VERSION >= "3.4.0" && RUBY_PLATFORM != "java" end # development dependencies for vscode integration and debugging diff --git a/Rakefile b/Rakefile index aa78fe8e..55c9cca7 100644 --- a/Rakefile +++ b/Rakefile @@ -159,6 +159,19 @@ namespace :spec do t.pattern = "spec/datadog/ci/git/**/*_spec.rb" t.rspec_opts = args.to_a.join(" ") end + + # ddcov test impact analysis tool with memcheck + desc "" + if Gem.loaded_specs.key?("ruby_memcheck") + RubyMemcheck::RSpec::RakeTask.new(:ddcov_memcheck) do |t, args| + t.pattern = "spec/ddcov/**/*_spec.rb" + t.rspec_opts = args.to_a.join(" ") + end + else + task :ddcov_memcheck do + raise "Memcheck requires the ruby_memcheck gem to be installed" + end + end end desc "CI task; it runs all tests for current version of Ruby" diff --git a/spec/datadog/ci/release_gem_spec.rb b/spec/datadog/ci/release_gem_spec.rb index 5708c587..785ccdd9 100644 --- a/spec/datadog/ci/release_gem_spec.rb +++ b/spec/datadog/ci/release_gem_spec.rb @@ -46,6 +46,7 @@ |tasks |yard |vendor/rbs + |suppressions )/ }x diff --git a/suppressions/ruby-3.4.supp b/suppressions/ruby-3.4.supp new file mode 100644 index 00000000..a5ae1ccf --- /dev/null +++ b/suppressions/ruby-3.4.supp @@ -0,0 +1,53 @@ +# This is a valgrind suppression configuration file. +# +# We use it together with the ruby_memcheck gem to find issues in the datadog-ci-rb native extensions; in some cases +# we need to ignore potential issues as they're not something we can fix (e.g. outside our code.) +# +# See https://valgrind.org/docs/manual/manual-core.html#manual-core.suppress for details. + +# When a Ruby process forks, it looks like Ruby doesn't clean up the memory of old threads? +{ + ruby-native-thread-memory + Memcheck:Leak + fun:calloc + fun:calloc1 + fun:rb_gc_impl_calloc + fun:native_thread_alloc + fun:native_thread_create_dedicated + fun:native_thread_create + fun:thread_create_core + ... +} + +# When a Ruby process forks, it looks like Ruby doesn't clean up the memory of old threads? +{ + ruby-native-thread-memory-2 + Memcheck:Leak + fun:calloc + fun:calloc1 + fun:objspace_xcalloc + fun:ruby_xcalloc_body + fun:native_thread_alloc + fun:native_thread_create_dedicated + fun:native_thread_create + fun:thread_create_core + ... +} + +# We don't care about the pkg-config external tool +{ + pkg-config-memory + Memcheck:Leak + ... + obj:/usr/bin/pkg-config + ... +} + +# We don't care about the tr external tool +{ + pkg-config-memory + Memcheck:Leak + ... + obj:/usr/bin/tr + ... +} From 1da4b42af73cee0f34aab07cd24b4e6c4cf86cf1 Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 8 Oct 2024 14:21:20 +0200 Subject: [PATCH 2/3] require and configure ruby_memcheck if the gem is present --- Rakefile | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Rakefile b/Rakefile index 55c9cca7..0a7caa65 100644 --- a/Rakefile +++ b/Rakefile @@ -7,6 +7,19 @@ require "rspec/core/rake_task" require "yard" require "rake/extensiontask" +if Gem.loaded_specs.key? "ruby_memcheck" + require "ruby_memcheck" + require "ruby_memcheck/rspec/rake_task" + + RubyMemcheck.config( + # If there's an error, print the suppression for that error, to allow us to easily skip such an error if it's + # a false-positive / something in the VM we can't fix. + valgrind_generate_suppressions: true, + # This feature provides better quality data -- I couldn't get good output out of ruby_memcheck without it. + use_only_ruby_free_at_exit: true + ) +end + RSpec::Core::RakeTask.new(:spec) Dir.glob("tasks/*.rake").each { |r| import r } From 157b0d820add9f618f06c53d3723d8e497a35cee Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 8 Oct 2024 14:35:07 +0200 Subject: [PATCH 3/3] add retries for valgrind installation --- .github/workflows/test-memory-leaks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-memory-leaks.yml b/.github/workflows/test-memory-leaks.yml index c5276ae7..eea8e858 100644 --- a/.github/workflows/test-memory-leaks.yml +++ b/.github/workflows/test-memory-leaks.yml @@ -11,5 +11,5 @@ jobs: bundler-cache: true # runs 'bundle install' and caches installed gems automatically bundler: latest cache-version: v1 # bump this to invalidate cache - - run: sudo apt install -y valgrind && valgrind --version + - run: sudo apt-get update && (sudo apt-get install -y valgrind || sleep 5 && sudo apt-get install -y valgrind) && valgrind --version - run: bundle exec rake compile spec:ddcov_memcheck