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

Prevent test suite lockups on test server failure #758

Merged
merged 1 commit into from Aug 27, 2021

Conversation

voxik
Copy link
Contributor

@voxik voxik commented Aug 26, 2021

This addresses two issues:

  1. When the test server fails for whatever reason, the test suite gets stuck in infinite loop reading stderr output. This can be simply prevented, because when the gets reads from closed pipe, it returns nil.

  2. When the test server fails during startup, there is no way to know why. Therefore the server messages are collected and reported in case the stderr pipe is unexpectedly closed.

Original behavior:

$ shindont
  
  Excon bad server interaction +++++  
  Excon basics +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon streaming basics #  
  Excon basics (Basic Auth Pass) +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics (Basic Auth Fail) +++  
  Excon basics (ssl) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics verify_hostname (ssl) ++  
  Excon ssl verify peer (ssl) ++++  
  Excon ssl verify peer (ssl cert store) ++  
  Excon basics (ssl file) (focus) +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics (ssl file paths) (focus) ++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+++++++++++++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+++++++++++++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+  
  Excon basics (ssl string) (focus) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics (Unix socket) #  
  Excon basics (reusable local port) ++  
  Excon Connection +++++++++++++++++++  
  HTTPStatusError request/response debugging ++++++++++++++  
  Excon response header support +++++++++++++++++++++  
  logging instrumentor +  
  Excon middleware   
  Excon support for middlewares that return canned responses ++  
  Excon redirecting with cookie preserved +  
  Excon Decompress Middleware ++++++++++++++++++  
  Excon Decompress Middleware ++  
  Excon request idempotencey +++++++++++++++  
  Excon instrumentation +++++++++++++++++++++++++++++++  
  Excon stubs +++++++++++++++++++++++++++++++++  
  Excon redirector support +  
  Excon redirector support with redirect loop +  
  Excon redirect support for relative Location headers +  
  Excon redirect support for relative Location headers with dot segments +  
  Excon redirecting post request +  
  Pipelined Requests ++  
  Excon proxy support ++++++++++++++++++++++++++++++++++++++++++++++++++++#  
  Excon query string variants +++++++++++  
  Excon request methods ++  
  Excon request methods +++++++  

where the process infinitely loops here:

excon/tests/test_helper.rb

Lines 247 to 251 in b64de99

def with_rackup(name, host="127.0.0.1")
pid, w, r, e = launch_process("rackup", "-s", "webrick", "--host", host, rackup_path(name))
until e.gets =~ /HTTPServer#start:/; end
yield
ensure

New behavior:

$ shindont
  
  Excon bad server interaction +++++  
  Excon basics +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon streaming basics #  
  Excon basics (Basic Auth Pass) +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics (Basic Auth Fail) +++  
  Excon basics (ssl) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics verify_hostname (ssl) ++  
  Excon ssl verify peer (ssl) ++++  
  Excon ssl verify peer (ssl cert store) ++  
  Excon basics (ssl file) (focus) +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics (ssl file paths) (focus) ++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+++++++++++++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+++++++++++++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
++++++++++:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+:certificate_path is no longer supported and will be deprecated. Please use :client_cert or :client_cert_data
:private_key_path is no longer supported and will be deprecated. Please use :client_key or :client_key_data
+  
  Excon basics (ssl string) (focus) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++  
  Excon basics (Unix socket) #  
  Excon basics (reusable local port) ++  
  Excon Connection +++++++++++++++++++  
  HTTPStatusError request/response debugging ++++++++++++++  
  Excon response header support +++++++++++++++++++++  
  logging instrumentor +  
  Excon middleware   
  Excon support for middlewares that return canned responses ++  
  Excon redirecting with cookie preserved +  
  Excon Decompress Middleware ++++++++++++++++++  
  Excon Decompress Middleware ++  
  Excon request idempotencey +++++++++++++++  
  Excon instrumentation +++++++++++++++++++++++++++++++  
  Excon stubs +++++++++++++++++++++++++++++++++  
  Excon redirector support +  
  Excon redirector support with redirect loop +  
  Excon redirect support for relative Location headers +  
  Excon redirect support for relative Location headers with dot segments +  
  Excon redirecting post request +  
  Pipelined Requests ++  
  Excon proxy support ++++++++++++++++++++++++++++++++++++++++++++++++++++#  
  Excon query string variants +++++++++++  
  Excon request methods ++  
  Excon request methods +++++++  
  Request Tests ++++      
      tests/request_tests.rb
        persistent connections
        Request Tests
      /usr/share/gems/gems/eventmachine-1.2.7/lib/eventmachine.rb:531:in `start_tcp_server': no acceptor (port is in use or requires root privileges) (RuntimeError)
	from /usr/share/gems/gems/eventmachine-1.2.7/lib/eventmachine.rb:531:in `start_server'
	from /builddir/build/BUILD/excon-0.85.0/usr/share/gems/gems/excon-0.85.0/tests/servers/good_ipv6.rb:6:in `block in <main>'
	from /usr/share/gems/gems/eventmachine-1.2.7/lib/eventmachine.rb:195:in `run_machine'
	from /usr/share/gems/gems/eventmachine-1.2.7/lib/eventmachine.rb:195:in `run'
	from /builddir/build/BUILD/excon-0.85.0/usr/share/gems/gems/excon-0.85.0/tests/servers/good_ipv6.rb:5:in `<main>'
 (RuntimeError)
        tests/test_helper.rb:303:in `with_server'
        tests/request_tests.rb:6:in `block (3 levels) in <top (required)>'
        tests/request_tests.rb:5:in `each'
        tests/request_tests.rb:5:in `block (2 levels) in <top (required)>'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:79:in `instance_eval'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:79:in `tests'
        tests/request_tests.rb:2:in `block in <top (required)>'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:79:in `instance_eval'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:79:in `tests'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:38:in `initialize'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:13:in `new'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo.rb:13:in `tests'
        tests/request_tests.rb:1:in `<top (required)>'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo/bin.rb:61:in `load'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo/bin.rb:61:in `block (2 levels) in run_in_thread'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo/bin.rb:58:in `each'
        /usr/share/gems/gems/shindo-0.3.10/lib/shindo/bin.rb:58:in `block in run_in_thread'
  
  Excon Response Parsing +++++++++++++++++++++++  
  Excon thread safety ++++  
  read should timeout ++  
  Excon::Utils +++++++++++++++  
  1 errored, 3 pending and 952 succeeded in 13.908285484560059 seconds

This addresses two issues:

1) When the test server fails for whatever reason, the test suite gets
   stuck in infinite loop reading stderr output. This can be simply
   prevented, because when the `gets` reads from closed pipe, it returns
   `nil`.

2) When the test server fails during startup, there is no way to know
   why. Therefore the server messages are collected and reported in case
   the stderr pipe is unexpectedly closed.
@voxik
Copy link
Contributor Author

voxik commented Aug 26, 2021

Please note that I have tried also more concise approach such as:

$ git diff
diff --git a/lib/excon/test/plugin/server/puma.rb b/lib/excon/test/plugin/server/puma.rb
index d87a8af..b8e1a27 100644
--- a/lib/excon/test/plugin/server/puma.rb
+++ b/lib/excon/test/plugin/server/puma.rb
@@ -5,9 +5,10 @@ module Excon
         module Puma
           def start(app_str = app, bind_uri = bind)
             open_process('puma', '-b', bind_uri.to_s, app_str)
-            line = ''
-            until line =~ /Use Ctrl-C to stop/
-              line = read.gets
+            process_stderr = ""
+            until (line = read.gets) =~ /Use Ctrl-C to stop/
+              raise process_stderr if line.nil?
+              process_stderr << line
               fatal_time = elapsed_time > timeout
               raise 'puma server has taken too long to start' if fatal_time
             end

But it was getting stuck for some strange reason.

Copy link
Contributor

@geemus geemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@voxik Looks good, thanks for digging into this, the detailed explanation and a fix. I appreciate the help!

@geemus geemus merged commit 4075233 into excon:master Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants