Skip to content

Refactor drain on shutdown#2600

Merged
MSP-Greg merged 2 commits intopuma:masterfrom
wjordan:refactor_drain_on_shutdown
Apr 30, 2021
Merged

Refactor drain on shutdown#2600
MSP-Greg merged 2 commits intopuma:masterfrom
wjordan:refactor_drain_on_shutdown

Conversation

@wjordan
Copy link
Copy Markdown
Contributor

@wjordan wjordan commented Apr 17, 2021 •
edited
Loading

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.

@wjordan wjordan mentioned this pull request Apr 17, 2021
Closed
7 tasks
MSP-Greg
MSP-Greg reviewed Apr 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@MSP-Greg MSP-Greg left a comment

Choose a reason for hiding this comment

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

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

@MSP-Greg
Copy link
Copy Markdown
Member

MSP-Greg commented Apr 17, 2021 •
edited
Loading

@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
Copy link
Copy Markdown
Contributor Author

wjordan commented Apr 17, 2021

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
Copy link
Copy Markdown
Member

MSP-Greg commented Apr 18, 2021

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 wjordan force-pushed the refactor_drain_on_shutdown branch from 2fa45e8 to 98df869 Compare April 18, 2021 05:26
@wjordan
Copy link
Copy Markdown
Contributor Author

wjordan commented Apr 18, 2021 •
edited
Loading

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...)

@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label Apr 18, 2021
@nateberkopec nateberkopec added this to the 5.3.0 milestone Apr 24, 2021
@wjordan wjordan force-pushed the refactor_drain_on_shutdown branch from 98df869 to 0689c04 Compare April 30, 2021 17:16
@wjordan wjordan force-pushed the refactor_drain_on_shutdown branch from 0689c04 to bb566d4 Compare April 30, 2021 18:06
@MSP-Greg MSP-Greg removed the waiting-for-review Waiting on review from anyone label Apr 30, 2021
@MSP-Greg MSP-Greg merged commit ff17194 into puma:master Apr 30, 2021
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
@wjordan @JuanitoFatas
Refactor drain on shutdown (puma#2600)
fca688b 
* Refactor test_puma_server#server_run for option arguments

* Refactor drain_on_shutdown into main server accept loop
Also add test coverage for drain_on_shutdown option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MSP-Greg MSP-Greg MSP-Greg left review comments

Assignees

No one assigned

Labels

refactor

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@wjordan @MSP-Greg @nateberkopec