[PR 1952] 2st verification - updated tests & GitHub Actions #1956

MSP-Greg · 2019-09-10T02:55:22Z

PR #1955 shows that the tests in this PR do not pass on current master.

Below is a summary of the changes here. The last commit adds two tests that use UNIXSockets for the bind address, verifying that unix files seem to be handled correctly during shutdowns and restarts.

lib files - binder.rb, cluster.rb, launcher.rb

binder.rb - Add PR fix Binder#close #1886
cluster.rb - Add PR Fix Worker external TERM signalling #1952, additional minor changes
launcher.rb - #close_binder_listeners - close io's as fast as possible. Unlink files in separate iteration.

test_integration.rb

Request handling during server TERM - two tests

#test_term_closes_listeners_cluster_tcp
#test_term_closes_listeners_cluster_unix

using #term_closes_listeners_cluster

Send requests 10 per second. Send 10, then :TERM server, then send another 30.
No more than 10 should throw Errno::ECONNRESET.

Request handling during phased restart - two tests

#test_usr1_all_respond_cluster_tcp
#test_usr1_all_respond_cluster_unix

using #usr1_all_respond_cluster

Send requests 1 per second. Send 1, then :USR1 server, then send another 24.
All should be responded to, and at least three workers should be used

Stuck worker tests - two tests

#test_stuck_external_term_spawn_cluster
Tests whether externally TERM'd 'stuck' workers are proper re-spawned.

#test_stuck_phased_restart_cluster
Tests whether 'stuck' workers are properly shutdown during phased-restart.

GitHub Actions - workflow.yml

Install ragel for Ubuntu and macOS. Both intermittently failed compile without doing so.
Windows - Add step to update MSYS2 gcc, install ragel and proper OpenSSL package.
Actions CI seems stable, and it finishes well before either Travis or AppVeyor.

Misc test files changes

Added parallelize_me! to a few files, misc changes

1. binder.rb Add PR puma#1886 2. cluster.rb Add PR puma#1952, additional minor changes 3. launcher.rb #close_binder_listeners close io's as fast as possible

1. test_integration.rb - Rewrite so all tests create a server via IO.popen. 2. Update some tests for parallelize_me! 3. Add config/worker_shutdown_timeout_2.rb, rackup/sleep_pid.ru

cluster.rb - clean up duplicate term? method workflow.yml - better step descriptions, add PR's

nateberkopec · 2019-09-11T14:11:00Z

test/test_puma_server.rb


  def setup
-    @port = 0
+    @port = UniquePort.call


Why can't we just use port = 0 everywhere, I wonder?

Good question. Only thing is that all the code would have to be changed for the 'clients', but we could set that in wait_for_server_to_boot?

I'm !AFK now, so as soon as the CI finishes, I'll have a look, as it's pretty 'fresh' in my mind...

nateberkopec · 2019-09-11T14:59:02Z

lib/puma/launcher.rb

@@ -217,11 +217,12 @@ def restart_args
    end

    def close_binder_listeners
-      @binder.listeners.each do |l, io|
-        io.close
+      # close binder listeners as fast as possible, so separate loop


We want the listener sockets closed quickly once shutdown starts. Anything that requires checking the disk can slow down. Small issue.

MSP-Greg · 2019-09-11T14:59:51Z

@nateberkopec

One typo I didn't explicitly mention:

puma/test/helpers/integration.rb

Lines 20 to 25 in 8e2c461

    
           def teardown 
        
             if defined?(@server) && @server 
        
               begin 
        
                 Process.kill "INT", @server.pid 
        
               rescue 
        
                 Errno::ESRCH

Line 25 should be combined with line 24. In case you've got another commit waiting.

nateberkopec · 2019-09-11T14:59:57Z

lib/puma/binder.rb

@@ -50,14 +50,6 @@ def env(sock)

    def close
      @ios.each { |i| i.close }
-      @unix_paths.each do |i|


This doesn't fix the problem, as bind paths are still not removed after program stop.

A few days ago, I was really clear on what was where in terms of Binder @ios, @unix_paths, and @listeners. I should have added some comments...

Paths are closed in Binder#close_listeners, They aren't in Binder#close

See this assertion I've added to master. Anything that purports to fix this problem needs to pass this test (which is failing on master).

nateberkopec · 2019-09-11T15:17:19Z

test/helper.rb

@@ -70,6 +74,34 @@ def run(*)

 module TestSkips

+  # finds bad signals, value is an array of symbols
+  BAD_SIGNAL_LIST = begin


Not a huge fan of this, what's wrong with Signal.list?

Windows Signal.list includes :TERM, but it generates an error when used. Wasn't sure if other OS's that aren't tested might also behave the same way.

Needed a process to kill to test for it...

I appreciate the idea, but let's just remove :TERM from the list if on Windows, this spawning-a-process thing feels like the nuclear option.

Also, if :TERM doesnt work on windows, why do tests that use not fail? e.g. test_term_closes_listeners or test_term_suppress

Of of course, because you can't fork on windows. D'oh!

let's just remove :TERM from the list if on Windows, this spawning-a-process thing feels like the nuclear option

Understandable.

I really hate defining by OS, as opposed to actually checking for functionality. Maybe it (:TERM) could change in a year or something. Probably unlikely...

nateberkopec · 2019-09-11T15:21:36Z

OK, let's try again with this:

1 commit that adds the tests described in the OP
1 commit each for the 3 changes described in the lib section of the OP (so, 3 more commits)
Save everything else, e.g. Github Actions, the signal test helper changes, etc for other PRs.

MSP-Greg · 2019-09-11T16:55:20Z

Of the above three items, '1 commit that adds the tests described in the OP' is the messiest. I'm happy to look at it or do the PR, but you may want to do it.

I was told I'd be receiving info about an ongoing project early this week, which will keep me busy for several days. Hence, the PR. I've now been told it will be later this week or next...

test_integration_cluster.rb Request handling during server TERM - two tests `#test_term_closes_listeners_cluster_tcp` `#test_term_closes_listeners_cluster_unix` using `#term_closes_listeners_cluster` Send requests 10 per second. Send 10, then :TERM server, then send another 30. No more than 10 should throw Errno::ECONNRESET. Request handling during phased restart - two tests `#test_usr1_all_respond_cluster_tcp` `#test_usr1_all_respond_cluster_unix` using `#usr1_all_respond_cluster` Send requests 1 per second. Send 1, then :USR1 server, then send another 24. All should be responded to, and at least three workers should be used Stuck worker tests - two tests `#test_stuck_external_term_spawn_cluster` Tests whether externally TERM'd 'stuck' workers are proper re-spawned. `#test_stuck_phased_restart_cluster` Tests whether 'stuck' workers are properly shutdown during phased-restart. helper files/methods changes 1. helper file changes to allow binding to TCP or UNIX, see kwarg unix: 2. Skip on Windows for signal TERM

test_integration_cluster.rb Request handling during server TERM - two tests `#test_term_closes_listeners_tcp` `#test_term_closes_listeners_unix` using `#term_closes_listeners` Send requests 10 per second. Send 10, then :TERM server, then send another 30. No more than 10 should throw Errno::ECONNRESET. Request handling during phased restart - two tests `#test_usr1_all_respond_tcp` `#test_usr1_all_respond_unix` using `#usr1_all_respond` Send requests 1 per second. Send 1, then :USR1 server, then send another 24. All should be responded to, and at least three workers should be used Stuck worker tests - two tests `#test_stuck_external_term_spawn` Tests whether externally TERM'd 'stuck' workers are proper re-spawned. `#test_stuck_phased_restart` Tests whether 'stuck' workers are properly shutdown during phased-restart. helper files/methods changes 1. helper file changes to allow binding to TCP or UNIX, see kwarg unix: 2. Skip on Windows for signal TERM

* Update test_integration files per PR #1956 test_integration_cluster.rb Request handling during server TERM - two tests `#test_term_closes_listeners_tcp` `#test_term_closes_listeners_unix` using `#term_closes_listeners` Send requests 10 per second. Send 10, then :TERM server, then send another 30. No more than 10 should throw Errno::ECONNRESET. Request handling during phased restart - two tests `#test_usr1_all_respond_tcp` `#test_usr1_all_respond_unix` using `#usr1_all_respond` Send requests 1 per second. Send 1, then :USR1 server, then send another 24. All should be responded to, and at least three workers should be used Stuck worker tests - two tests `#test_stuck_external_term_spawn` Tests whether externally TERM'd 'stuck' workers are proper re-spawned. `#test_stuck_phased_restart` Tests whether 'stuck' workers are properly shutdown during phased-restart. helper files/methods changes 1. helper file changes to allow binding to TCP or UNIX, see kwarg unix: 2. Skip on Windows for signal TERM * Misc updates, debug output, cleanup * Add comments * fix test_int_signal_with_background_thread_in_jruby per review * TestIntegrationCluster#term_closes_listeners - add interleaved assert * cluster.rb - remove duplicate Worker#term? method

MSP-Greg added 5 commits September 10, 2019 11:03

Updates binder.rb, cluster.rb, launcher.rb

4471054

1. binder.rb Add PR puma#1886 2. cluster.rb Add PR puma#1952, additional minor changes 3. launcher.rb #close_binder_listeners close io's as fast as possible

Add GitHub Actions workflow.yml

8d1d394

Update tests

e907a0e

1. test_integration.rb - Rewrite so all tests create a server via IO.popen. 2. Update some tests for parallelize_me! 3. Add config/worker_shutdown_timeout_2.rb, rackup/sleep_pid.ru

Travis macOS - Ruby 2.4.7 not available yet, revert to 2.4.6

259d565

test_integration.rb - add two tests using UNIX binders

ea2c264

cluster.rb - clean up duplicate term? method workflow.yml - better step descriptions, add PR's

MSP-Greg force-pushed the update-lib-test-actions branch from ba6502d to ea2c264 Compare September 10, 2019 16:06

nateberkopec reviewed Sep 11, 2019

View reviewed changes

nateberkopec mentioned this pull request Sep 11, 2019

Fix Worker external TERM signalling #1952

Merged

nateberkopec reviewed Sep 11, 2019

View reviewed changes

nateberkopec closed this Sep 11, 2019

MSP-Greg mentioned this pull request Sep 11, 2019

Try Github Actions #1918

Merged

6 tasks

MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 13, 2019

Update test_integration files per PR puma#1956

e566a1f

MSP-Greg mentioned this pull request Sep 13, 2019

Update test_integration files per PR #1956 #1965

Merged

MSP-Greg deleted the update-lib-test-actions branch September 18, 2019 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR 1952] 2st verification - updated tests & GitHub Actions #1956

[PR 1952] 2st verification - updated tests & GitHub Actions #1956

MSP-Greg commented Sep 10, 2019 •

edited

nateberkopec Sep 11, 2019

MSP-Greg Sep 11, 2019

nateberkopec Sep 11, 2019

MSP-Greg Sep 11, 2019

MSP-Greg commented Sep 11, 2019

nateberkopec Sep 11, 2019

MSP-Greg Sep 11, 2019

nateberkopec Sep 11, 2019

nateberkopec Sep 11, 2019

MSP-Greg Sep 11, 2019 •

edited

nateberkopec Sep 11, 2019

nateberkopec Sep 11, 2019

nateberkopec Sep 11, 2019

MSP-Greg Sep 11, 2019

nateberkopec commented Sep 11, 2019

MSP-Greg commented Sep 11, 2019

[PR 1952] 2st verification - updated tests & GitHub Actions #1956

[PR 1952] 2st verification - updated tests & GitHub Actions #1956

Conversation

MSP-Greg commented Sep 10, 2019 • edited

lib files - binder.rb, cluster.rb, launcher.rb

test_integration.rb

Request handling during server TERM - two tests

Request handling during phased restart - two tests

Stuck worker tests - two tests

GitHub Actions - workflow.yml

Misc test files changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg Sep 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateberkopec commented Sep 11, 2019

MSP-Greg commented Sep 11, 2019

MSP-Greg commented Sep 10, 2019 •

edited

MSP-Greg Sep 11, 2019 •

edited