@dentarg
Thank you for your review.
I will follow your comments and make the changes.

@kos31de First of all, good idea, and thanks for working on it.

In general, not sure changes to the test or lib folders will require this to be run. Or, only changes to the ext folder are what should be checked? It might be better to either add this to the ragel workflow file (which is filtered on the ext path), or make it a separate file with path filtering.

Secondly, based on @dentarg's comments, if bundle install is done in setup-ruby, it should be changed to setup-ruby-pkgs so valgrind is installed before ruby_memcheck, the input would be apt-get: ragel valgrind.

EDIT: Got it running locally, although tests are running twice. Not.sure.why

But, there are some things reported, making changes , mostly to minissl.c...

Thank you so much for taking this up @kos31de !

Thanks for working on this @kos31de! Feel free to reach out if you need any help!

Thank you all for your comments.
I'll try running it locally first and see if there are any problems.

I have found a problem of the test being run twice.
I am currently working to resolve this issue.

# Running:

...........S......................S...............EEEEEES.SSS......................................................................................................E...................................................................................................SS...SSS.S.S....S...S..SS.....SS..S...................................../usr/lib/ruby/3.0.0/uri/common.rb:286:in `chr': Interrupt
	from /usr/lib/ruby/3.0.0/uri/common.rb:286:in `block in <module:URI>'
	from /usr/lib/ruby/3.0.0/uri/common.rb:284:in `times'
	from /usr/lib/ruby/3.0.0/uri/common.rb:284:in `<module:URI>'
	from /usr/lib/ruby/3.0.0/uri/common.rb:15:in `<top (required)>'
	from /usr/lib/ruby/3.0.0/uri.rb:95:in `require_relative'
	from /usr/lib/ruby/3.0.0/uri.rb:95:in `<top (required)>'
	from <internal:/usr/lib/ruby/vendor_ruby/rubygems/core_ext/kernel_require.rb>:85:in `require'
	from <internal:/usr/lib/ruby/vendor_ruby/rubygems/core_ext/kernel_require.rb>:85:in `require'
	from /usr/lib/ruby/vendor_ruby/rubygems/specification_policy.rb:408:in `validate_lazy_metadata'
	from /usr/lib/ruby/vendor_ruby/rubygems/specification_policy.rb:89:in `validate_required!'
	from /usr/lib/ruby/vendor_ruby/rubygems/specification_policy.rb:44:in `validate'
	from /usr/lib/ruby/vendor_ruby/rubygems/specification.rb:2583:in `validate'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/rubygems_integration.rb:68:in `block in validate'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/ui/shell.rb:136:in `with_level'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/ui/shell.rb:88:in `silence'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/rubygems_integration.rb:68:in `validate'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/source/path.rb:168:in `validate_spec'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/source/path.rb:182:in `block in load_spec_files'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/source/path.rb:176:in `each'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/source/path.rb:176:in `load_spec_files'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/source/path.rb:107:in `local_specs'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/source/path.rb:115:in `specs'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:615:in `specs_for_source_changed?'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:600:in `specs_changed?'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:644:in `block in converge_paths'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:643:in `any?'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:643:in `converge_paths'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:135:in `initialize'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/dsl.rb:219:in `new'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/dsl.rb:219:in `to_definition'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/dsl.rb:13:in `evaluate'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/definition.rb:38:in `build'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler.rb:208:in `definition'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler.rb:156:in `setup'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/setup.rb:20:in `block in <top (required)>'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/ui/shell.rb:136:in `with_level'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/ui/shell.rb:88:in `silence'
	from /var/lib/gems/3.0.0/gems/bundler-2.3.22/lib/bundler/setup.rb:20:in `<top (required)>'
	from <internal:/usr/lib/ruby/vendor_ruby/rubygems/core_ext/kernel_require.rb>:85:in `require'
	from <internal:/usr/lib/ruby/vendor_ruby/rubygems/core_ext/kernel_require.rb>:85:in `require'
#<Thread:0x000000000babe7c0 /home/ubuntu/puma/test/helpers/integration.rb:340 run> terminated with exception (report_on_exception is true):
/home/ubuntu/puma/test/helpers/integration.rb:136:in `gets': stream closed in another thread (IOError)
	from /home/ubuntu/puma/test/helpers/integration.rb:136:in `wait_for_server_to_boot'
	from /home/ubuntu/puma/test/helpers/integration.rb:349:in `block in hot_restart_does_not_drop_connections'
/home/ubuntu/puma/test/helpers/integration.rb:136:in `gets': stream closed in another thread (IOError)
	from /home/ubuntu/puma/test/helpers/integration.rb:136:in `wait_for_server_to_boot'
	from /home/ubuntu/puma/test/helpers/integration.rb:349:in `block in hot_restart_does_not_drop_connections'
E.......E..................EEEE.EE.......SS.....SS...S.S..S.S....S.S..SSS.....S..E...S.E.../home/ubuntu/puma/lib/puma/log_writer.rb:61:in `write': deadlock; recursive locking (ThreadError)

@kos31de

Thanks for working on this.

It might be better to have a positive ENV variable like PUMA_MEMCHECK rather than PUMA_NO_MEMCHECK. It needs a 'guard' in both the Gemfile and the Rakefile.

Locally, I threw something together, I used PUMA_RUBY_MEMCHECK. See MSP-Greg@4d910cfae34e

I was only running tests with test_puma_server_ssl.rb. The 'integration' tests may be interesting. I'm not sure if ruby_memcheck can pick up data from the server instances run with IO.popen. That has also been an issue with code coverage systems...

AFAIK IO.popen runs a forked process? ruby_memcheck should correctly handle forks. It configures valgrind to follow all forked children.

https://github.com/Shopify/ruby_memcheck/blob/810e5a41ddc7698d504f0ec9a2f1b19a1a26abf4/lib/ruby_memcheck/configuration.rb#L9

@peterzhu2118

Thanks for helping, and also for ruby_memcheck.

AFAIK IO.popen runs a forked process?

forked vs sub-process? Not sure. I'll check a bit later. Some tests also restart Puma with a different Gemfile, these may need to be skipped. The extension code in Puma is for the request http parser and ssl connections. Everything else is Ruby.

@kos31de

The below:

should have two changes (lines 186 & 190):

      - uses: ruby/setup-ruby-pkgs@v1
        with:
          ruby-version: '3.0'
          bundler-cache: true
          apt-get: ragel valgrind

@MSP-Greg
Thank you for all your helpful comments.
I have made the changes according to your comments.
fee4e6e

Due to the very long run times of the CI, I would try to minimise the impact, e.g. by filtering by paths.

During RubyKaigi, I discussed the issue of long completion times in CI when running with ruby_memcheck with @nateberkopec. He agreed that a good solution is to only run the ruby_memcheck pipeline only on the main branch, that way it doesn't block PRs while still running for every commit.

Maybe a bit of an explanation of setup-ruby vs setup-ruby-pkgs.

setup-ruby-pkgs allows simple cross-platform package installation, which can involve a lot of conditional code if done without it. It wraps the code in setup-ruby and includes all of setup-ruby's inputs.

Critically, it installs packages before setup-ruby runs bundle install, which is run if bundler-cache is true. So, if one is not using setup-ruby-pkgs and bundler-cache is true, one must install packages dependencies needed for bundle install before running setup-ruby.

Since this code will run only on Actions Ubuntu, it could be done with setup-ruby, but it's a bit more condensed with setup-ruby-pkgs, and also similar to the other cross-platform jobs.

a good solution is to only run the ruby_memcheck pipeline only on the main branch

Which makes it a total PITA to run in a fork. One of the main remaining issues with GitHub Actions.

I wonder if a few tests or scripts specific to ruby_memcheck should be used for CI. CI systems are limited, and some of the current tests (like the bundler tests for app reloading) may require patching to run with ruby_memcheck. Also, some tests are time sensitive/limited, and they may have issues running with ruby_memcheck.

Maybe send a stream of tcp sockets, and another with a stream of ssl sockets. That would check normal operation, maybe more code should be used to check if any error handling causes memory leaks.

I would like to get ruby_memcheck running as part of Puma's CI. I think it's probably a nontrivial task to properly implement.

@kos31de

Above I mentioned timing. I've been running Puma CI quite a bit this weekend.

Earlier, an Ubuntu system took 25 seconds to compile, most of the other Ubuntu jobs take 4 to 6 seconds. Just now, a macOS system took about 110 seconds to compile, normal times are around 11 seconds. The macOS system also failed the tests.

With variances like the above, CI stability can be very difficult. Adding valgrind to it (as is) would probably lead to insanity...

@MSP-Greg
Thank you for your comment.
I would like to reduce the execution time, but at the moment I do not have an effective idea.
I would consider forgoing the introduction of valgrind for now.

@kos31de

Thank you again for the PR, and thanks to @peterzhu2118 for participating.

Been busy re Puma 6.0.0, I need to work on some other projects. I want to pick up work on using ruby_memcheck/valgrind in CI sometime soon. I suspect writing code specifically for it is probably best. Filtering similar to the ragel workflow would only run CI with it when needed.

The extension code in Puma handles request parsing and SSL connections. Code that generates request streams is probably a good start, followed with code that throws some errors to check whether error handling has any leaks.

I would consider forgoing the introduction of valgrind for now.

Whatever you prefer. Leaving it open is fine by me, not speaking for others...

Thanks for your contribution @kos31de. I generally don't like leaving PRs open if we don't have a clear action to take at the moment. Please re-open whenever you think it is ready.