From 0b737cce42746d8226701520f35685e0624e061b Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 24 Mar 2020 22:13:31 +0100 Subject: [PATCH] Run tests on TruffleRuby, all tests pass now (#2198) * Support skip_on :truffleruby * Remove unused variable declaration * Properly skip tests which need fork * Improve NO_FORK_MSG * Keep the Tempfile instances alive in test_redirect_io.rb * Otherwise they could GC in the middle of the test, and the files could then be deleted. * Use a better way to find a free port * Read directly from the socket in #read_and_drop * There is no point to decode the bytes since we are closing the socket in Puma::MiniSSL::Socket#close. * Also, calling #engine_read_all might cause further SSL errors, which could hide the first SSL error. This notably happens in TestPumaServerSSLClient#test_verify_fail_if_no_client_cert if the server is faster than the client. The error in that case is "System error: Success - 0 (Puma::MiniSSL::SSLError)" which is not actually an error, but there is also nothing to read further from SSL. * TruffleRuby should pass the CI now, remove from allowed failures * Use a timeout of 120 for all non-MRI implementations * 60 doesn't seem enough in CI for TestThreadPool#test_trim on TruffleRuby. * Fix check for cluster mode in integration tests * Improve integration tests to fail more clearly if the pid file does not exist * Make integration tests more robust * Add skips for unreliable or racy tests * Add ChangeLog entry * No need to run RuboCop on non-MRI implementations * This should speed up CI a bit for those jobs. --- .github/workflows/ruby.yml | 13 ++++++++----- History.md | 1 + lib/puma/minissl.rb | 13 ++++++++----- test/helper.rb | 17 ++++++----------- test/shell/run.rb | 6 +++--- test/shell/t1.rb | 6 +++--- test/shell/t2.rb | 4 ++-- test/shell/t3.rb | 4 ++-- test/test_integration_pumactl.rb | 2 +- test/test_redirect_io.rb | 11 ++++++++--- test/test_thread_pool.rb | 3 ++- 11 files changed, 44 insertions(+), 36 deletions(-) diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml index 7561b03f..2beecd0d 100644 --- a/.github/workflows/ruby.yml +++ b/.github/workflows/ruby.yml @@ -18,7 +18,7 @@ jobs: fail-fast: false matrix: os: [ ubuntu-18.04, macos ] - ruby: [ 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, head, jruby ] + ruby: [ 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, head, jruby, truffleruby-head ] steps: - name: repo checkout @@ -42,9 +42,13 @@ jobs: - name: compile run: bundle exec rake compile + - name: rubocop + if: startsWith(matrix.ruby, '2.') + run: bundle exec rake rubocop + - name: test timeout-minutes: 8 - run: bundle exec rake + run: bundle exec rake test:all win32: name: >- @@ -89,7 +93,7 @@ jobs: timeout-minutes: 8 run: bundle exec rake - nonMRIHead: + allowedFailures: name: >- ${{ matrix.cfg.os }}, ${{ matrix.cfg.ruby }} env: @@ -104,8 +108,7 @@ jobs: fail-fast: false matrix: cfg: - - { os: ubuntu-latest, ruby: jruby-head } - - { os: ubuntu-latest, ruby: truffleruby-head } + - { os: ubuntu-latest, ruby: jruby-head } steps: - name: repo checkout diff --git a/History.md b/History.md index 47be42fb..12fd6b5e 100644 --- a/History.md +++ b/History.md @@ -25,6 +25,7 @@ * Rescue IO::WaitReadable instead of EAGAIN for blocking read (#2121) * Ensure `BUNDLE_GEMFILE` is unspecified in workers if unspecified in master when using `prune_bundler` (#2154) * Rescue and log exceptions in hooks defined by users (on_worker_boot, after_worker_fork etc) (#1551) + * Read directly from the socket in #read_and_drop to avoid raising further SSL errors (#2198) * Refactor * Remove unused loader argument from Plugin initializer (#2095) diff --git a/lib/puma/minissl.rb b/lib/puma/minissl.rb index c4b83b05..92ce973b 100644 --- a/lib/puma/minissl.rb +++ b/lib/puma/minissl.rb @@ -125,11 +125,14 @@ module Puma def read_and_drop(timeout = 1) return :timeout unless IO.select([@socket], nil, nil, timeout) - return :eof unless read_nonblock(1024) - :drop - rescue Errno::EAGAIN - # do nothing - :eagain + case @socket.read_nonblock(1024, exception: false) + when nil + :eof + when :wait_readable + :eagain + else + :drop + end end def should_drop_bytes? diff --git a/test/helper.rb b/test/helper.rb index b8b90d64..97a08724 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -43,15 +43,10 @@ def hit(uris) end module UniquePort - @port = 3211 - @mutex = Mutex.new - def self.call - @mutex.synchronize { - @port += 1 - @port = 3307 if @port == 3306 # MySQL on Actions - @port - } + TCPServer.open('127.0.0.1', 0) do |server| + server.connect_address.ip_port + end end end @@ -62,7 +57,7 @@ module TimeoutEveryTestCase end def run(*) - ::Timeout.timeout(Puma.jruby? ? 120 : 60, TestTookTooLong) { super } + ::Timeout.timeout(RUBY_ENGINE == 'ruby' ? 60 : 120, TestTookTooLong) { super } end end @@ -78,7 +73,7 @@ module TestSkips # usage: skip NO_FORK_MSG unless HAS_FORK # windows >= 2.6 fork is not defined, < 2.6 fork raises NotImplementedError HAS_FORK = ::Process.respond_to? :fork - NO_FORK_MSG = "Kernel.fork isn't available on the #{RUBY_PLATFORM} platform" + NO_FORK_MSG = "Kernel.fork isn't available on #{RUBY_ENGINE} on #{RUBY_PLATFORM}" # socket is required by puma # usage: skip UNIX_SKT_MSG unless UNIX_SKT_EXIST @@ -99,11 +94,11 @@ module TestSkips # optional suffix kwarg is appended to the skip message # optional suffix bt should generally not used def skip_on(*engs, suffix: '', bt: caller) - skip_msg = false engs.each do |eng| skip_msg = case eng when :darwin then "Skipped on darwin#{suffix}" if RUBY_PLATFORM[/darwin/] when :jruby then "Skipped on JRuby#{suffix}" if Puma.jruby? + when :truffleruby then "Skipped on TruffleRuby#{suffix}" if RUBY_ENGINE == "truffleruby" when :windows then "Skipped on Windows#{suffix}" if Puma.windows? when :ci then "Skipped on ENV['CI']#{suffix}" if ENV["CI"] when :no_bundler then "Skipped w/o Bundler#{suffix}" if !defined?(Bundler) diff --git a/test/shell/run.rb b/test/shell/run.rb index b25ea126..d5e9019f 100644 --- a/test/shell/run.rb +++ b/test/shell/run.rb @@ -1,10 +1,10 @@ require "puma" require "puma/detect" -TESTS_TO_RUN = if Puma.jruby? - %w[t1 t2] -else +TESTS_TO_RUN = if Process.respond_to?(:fork) %w[t1 t2 t3] +else + %w[t1 t2] end results = TESTS_TO_RUN.map do |test| diff --git a/test/shell/t1.rb b/test/shell/t1.rb index f1e72761..bffbfb47 100644 --- a/test/shell/t1.rb +++ b/test/shell/t1.rb @@ -1,8 +1,8 @@ system "ruby -rrubygems -Ilib bin/puma -p 10102 -C test/shell/t1_conf.rb test/rackup/hello.ru &" -sleep (defined?(JRUBY_VERSION) ? 7 : 5) -system "curl http://localhost:10102/" -system "kill `cat t1-pid`" +sleep 1 until system "curl http://localhost:10102/" + +Process.kill :TERM, Integer(File.read("t1-pid")) sleep 1 diff --git a/test/shell/t2.rb b/test/shell/t2.rb index fbcd143d..c76a8b20 100644 --- a/test/shell/t2.rb +++ b/test/shell/t2.rb @@ -1,6 +1,6 @@ system "ruby -rrubygems -Ilib bin/pumactl -F test/shell/t2_conf.rb start &" -sleep (defined?(JRUBY_VERSION) ? 7 : 5) -system "curl http://localhost:10103/" + +sleep 1 until system "curl http://localhost:10103/" out=`ruby -rrubygems -Ilib bin/pumactl -F test/shell/t2_conf.rb status` diff --git a/test/shell/t3.rb b/test/shell/t3.rb index 9b0dafd7..125a07d6 100644 --- a/test/shell/t3.rb +++ b/test/shell/t3.rb @@ -3,13 +3,13 @@ sleep 5 worker_pid_was_present = File.file? "t3-worker-2-pid" -system "kill `cat t3-worker-2-pid`" # kill off a worker +Process.kill :TERM, Integer(File.read("t3-worker-2-pid")) # kill off a worker sleep 2 worker_index_within_number_of_workers = !File.file?("t3-worker-3-pid") -system "kill `cat t3-pid`" +Process.kill :TERM, Integer(File.read("t3-pid")) File.unlink "t3-pid" if File.file? "t3-pid" File.unlink "t3-worker-0-pid" if File.file? "t3-worker-0-pid" diff --git a/test/test_integration_pumactl.rb b/test/test_integration_pumactl.rb index 6add931f..c29c9a6b 100644 --- a/test/test_integration_pumactl.rb +++ b/test/test_integration_pumactl.rb @@ -20,7 +20,7 @@ class TestIntegrationPumactl < TestIntegration end def test_stop_tcp - skip_on :jruby # Undiagnose thread race. TODO fix + skip_on :jruby, :truffleruby # Undiagnose thread race. TODO fix @control_tcp_port = UniquePort.call cli_server "-q test/rackup/sleep.ru --control-url tcp://#{HOST}:#{@control_tcp_port} --control-token #{TOKEN} -S #{@state_path}" diff --git a/test/test_redirect_io.rb b/test/test_redirect_io.rb index cee3ba39..7600aa16 100644 --- a/test/test_redirect_io.rb +++ b/test/test_redirect_io.rb @@ -7,8 +7,11 @@ class TestRedirectIO < TestIntegration def setup super - @out_file_path = Tempfile.new('puma-out').path - @err_file_path = Tempfile.new('puma-err').path + # Keep the Tempfile instances alive to avoid being GC'd + @out_file = Tempfile.new('puma-out') + @err_file = Tempfile.new('puma-err') + @out_file_path = @out_file.path + @err_file_path = @err_file.path end def teardown @@ -16,6 +19,8 @@ class TestRedirectIO < TestIntegration paths = [@out_file_path, @err_file_path, @old_out_file_path, @old_err_file_path].compact File.unlink(*paths) + @out_file = nil + @err_file = nil end def test_sighup_redirects_io_single @@ -47,7 +52,7 @@ class TestRedirectIO < TestIntegration end def test_sighup_redirects_io_cluster - skip_on :jruby # Server isn't coming up in CI, TODO Fix + skip NO_FORK_MSG unless HAS_FORK skip_unless_signal_exist? :HUP cli_args = [ diff --git a/test/test_thread_pool.rb b/test/test_thread_pool.rb index bb562885..a1312fd6 100644 --- a/test/test_thread_pool.rb +++ b/test/test_thread_pool.rb @@ -64,7 +64,7 @@ class TestThreadPool < Minitest::Test end def test_trim - skip_on :jruby # Undiagnose thread race. TODO fix + skip_on :jruby, :truffleruby # Undiagnose thread race. TODO fix pool = new_pool(0, 1) do |work| @work_mutex.synchronize do @work_done.signal @@ -87,6 +87,7 @@ class TestThreadPool < Minitest::Test end def test_trim_leaves_min + skip_on :truffleruby # Undiagnose thread race. TODO fix pool = new_pool(1, 2) do |work| @work_mutex.synchronize do @work_done.signal