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

Add hot restart integration test [changelog skip] #2417

Closed
wants to merge 2 commits into from

Conversation

cjlarose
Copy link
Member

@cjlarose cjlarose commented Oct 7, 2020

Description

I created https://github.com/cjlarose/puma-phased-restart-errors as a way to demonstrate connection errors related to hot restarts, phased restarts, and shutdowns (documented in #2337). The project has been helpful in catching concurrency errors such as in #2279 (comment). It has been helpful, too, in verifying fixes like #2377 and #2122.

This PR introduces an integration test that aims to serve a similar purpose. I hope that it'll help prevent regressions related to connection handling and concurrency, especially around hot restarts, phased restarts, and shutdowns. By putting it in the repo along with the normal test suite, we can identify failures more quickly and easily.

The test basically just hammers a puma single-mode cluster while concurrently performing a bunch of hot restarts. The expectation is that all clients receive successful responses (all accepted connections on the socket are responded to as expected).

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added 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.

This test demonstrates that it's possible for clients to receive
unexpected ECONNRESET errors during hot restarts.

We skip the test on JRuby since connection reset errors are expected on
that platform because we cannot reliably pass open file descriptors
(such as the listening sockets) to the new process after a hot restart.

The test is skipped on TruffleRuby as well since on that implementation,
the test fails with an IOError when trying to restart the server.
@nateberkopec
Copy link
Member

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Oct 8, 2020
@dentarg
Copy link
Member

dentarg commented Oct 9, 2020

Looks like it may not be stable enough?

I tried to understand that failure... is it failing because of the 45 seconds test case limit? Or is it something else?

Another, more general questions about our tests: are test timeouts retried by minitest or does that only apply to other failures?

@MSP-Greg
Copy link
Member

MSP-Greg commented Oct 9, 2020

@dentarg

is it failing because of the 45 seconds test case limit? Or is it something else?

I think the timeout issues issue is caused by the timeout in read_body:

def read_body(connection, time_out = 10)
Timeout.timeout(time_out) do
loop do
response = connection.readpartial(1024)
body = response.split("\r\n\r\n", 2).last
return body if body && !body.empty?
sleep 0.01
end
end
end

test timeouts retried by minitest or does that only apply to other failures?

I believe any failure (including timeouts) is retried. But, I'm confused about the console output, as it's a bit unclear...

@cjlarose
Copy link
Member Author

Closing in favor of #2423

@cjlarose cjlarose closed this Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants