Refactor drain on shutdown

[wjordan]

Description

This refactoring PR moves the drain_on_shutdown implementation out of #graceful_shutdown to reduce duplication with the main server accept loop, including a test covering the basic behavior of this option to prevent regressions.

There might be some slight change in timing around when exactly the :state event gets fired (before or after connections are all drained), but this doesn't seem important enough to worry about (there was zero test coverage around it).

• dad2708 refactors TestPumaServer#server_run, in order to allow arbitrary option arguments to be passed. This made writing the added test with a custom option a bit easier and more readable, and allowed me to clean up a bit of boilerplate in the rest of the test suite.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[MSP-Greg]

Any idea about the Ruby 2.2 fails? Otherwise, LGTM.

[MSP-Greg]

@wjordan

Maybe change the test to the following? Passed in my fork...

def test_drain_on_shutdown
  server_run(drain_on_shutdown: true, max_threads: 1) do
    sleep 0.005
    [200, {}, ["DONE"]]
  end
  connections = Array.new(20) { send_http "GET / HTTP/1.0\r\n\r\n" }
  @server.stop
  @thread.join
  connections.each do |s|
    assert_match 'DONE', s.read
  end
end

def test_not_drain_on_shutdown
  server_run(max_threads: 1) do
    sleep 0.005
    [200, {}, ["DONE"]]
  end
  connections = Array.new(20) { send_http "GET / HTTP/1.0\r\n\r\n" }
  @server.stop
  @thread.join
  bad = 0
  connections.each do |s|
    begin
      assert_match 'DONE', s.read
    rescue Errno::ECONNRESET
      bad += 1
    end
  end
  assert_operator bad, :>, 5
end

EDIT: "Greg, what is @thread?"

Attached patch file (remove .txt from file name)..

test_puma_server.rb.patch.txt

[wjordan]

Any idea about the Ruby 2.2 fails?

Oops! Queue#close was added in Ruby 2.3, I had earlier written a simple backport patch (Puma::QueueClose) but the simple patch did not un-block already-waiting threads when close was called as expected. I added a few extra lines to the patch which fixes the test on Ruby 2.2. Thanks for taking a look!

I also added a test_not_drain_on_shutdown based on your patch, thanks!

[MSP-Greg]

Please have a look at the errors in the 2.2 test logs. I added the sleep 0.005 as it only adds 100 mS to each test, and pretty much forces the behavior we want, regardless of issues with test runners, etc, etc... Or, I think the test_drain_on_shutdown test in the patch should work in CI.

Something is amiss with my Windows Ruby 2.2 (build system) on the new desktop, I can't build Puma, so I just used CI...

[wjordan]

Please have a look at the errors in the 2.2 test logs.

Oops again- I messed up the QueueClose#pop implementation (returns nil even if there are items in the closed queue), fixed. (Hope Ruby 2.2 support can be dropped soon...)