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

Better error handling during force shutdown #2271

Merged
merged 1 commit into from Sep 24, 2020

Conversation

@wjordan
Copy link
Contributor

@wjordan wjordan commented May 17, 2020

Description

Refactors tests covering shutdown behavior to be less flaky (the current tests depend on fragile sleep timing that isn't 100% deterministic), and also fixes a couple of subtle timing edge-cases around force-shutdown behavior:

  • Only allow ForceShutdown to be raised in a thread during specific areas of the connection-processing cycle (marked by with_force_shutdown blocks), to ensure that the raised error is always rescued and handled cleanly. (This fixes an issue where the force_shutdown_after: 0 option throws uncaught exceptions from the threadpool on shutdown)
  • Add @queue_mutex and helper methods if_queue_requests and queue_client to Server to handle an edge case where the reactor shuts down while a thread is concurrently queuing a client. (#2337) A fix for this issue was merged in #2377.

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] 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.
@wjordan wjordan force-pushed the wjordan:force_shutdown_timing branch from b0626f0 to a290f14 May 17, 2020
@@ -244,19 +244,17 @@ def eagerly_finish
end # IS_JRUBY

def finish(timeout)
return true if @ready

This comment has been minimized.

@nateberkopec

nateberkopec Jun 10, 2020
Member

Hm, what's the impact of changing the return value here? Any?

This comment has been minimized.

@nateberkopec

nateberkopec Jun 10, 2020
Member

Just looking through the rest of the code, seems like we never checked the value.

# Returns block return value if queue is enabled, or nil if queue is not enabled.
def if_queue_requests
@queue_mutex.synchronize do
if @queue_requests

This comment has been minimized.

@nateberkopec

nateberkopec Jun 10, 2020
Member

Since queue_requests is set at configure-time, I think we can move this outside the mutex, right?

def queue_client(client, timeout)
if_queue_requests do
client.set_timeout timeout
@reactor.add client

This comment has been minimized.

@nateberkopec

nateberkopec Jun 10, 2020
Member

Would it be possible to keep concurrency as a concern of the Reactor rather than the Server? It sort of feels like we could refactor this a little further to keep concurrency concerns out of this class.

@nateberkopec
Copy link
Member

@nateberkopec nateberkopec commented Jun 10, 2020

I think my only concerns are the concurrency-awareness being added to Server when I think it hasn't had many concurrency-related reponsibilities yet. I'm wondering if we can shove all of that into Reactor instead so we keep Puma's main "concurrency-aware" classes as Reactor and Threadpool.

@wjordan
Copy link
Contributor Author

@wjordan wjordan commented Jun 10, 2020

I'm wondering if we can shove all of that into Reactor instead so we keep Puma's main "concurrency-aware" classes as Reactor and Threadpool.

I agree, the Reactor refactoring in #2279 updates Reactor#add to better handle this edge-case where #shutdown is concurrently called, so synchronization would no longer be needed in Server itself.

If that Reactor refactoring seems reasonable, this PR might be simpler following that one (e.g., 9975355.)

@nateberkopec
Copy link
Member

@nateberkopec nateberkopec commented Jun 11, 2020

Sounds good, we can block this PR on that one.

Only allow `ForceShutdown` to be raised in a thread during specific areas of the
connection-processing cycle (marked by `with_force_shutdown` blocks),
to ensure that the raised error is always rescued and handled cleanly.
Fixes an issue where the `force_shutdown_after: 0` option throws
uncaught exceptions from the threadpool on shutdown.
@wjordan wjordan force-pushed the wjordan:force_shutdown_timing branch from a290f14 to fcfe784 Sep 23, 2020
@wjordan
Copy link
Contributor Author

@wjordan wjordan commented Sep 23, 2020

Rebased after #2377 was merged, which fixed the second issue addressed by this PR (I've crossed out that part in the PR description above). This also means that the remaining part of this PR is no longer blocked on #2279 so it's ready for another review.

@nateberkopec nateberkopec merged commit f4c59a0 into puma:master Sep 24, 2020
30 of 33 checks passed
30 of 33 checks passed
ubuntu-20.04 2.5
Details
build
Details
ubuntu-20.04 2.7
Details
ubuntu-20.04 jruby ubuntu-20.04 jruby
Details
ubuntu-20.04 2.7
Details
ubuntu-20.04 head ubuntu-20.04 head
Details
windows-2019 2.7
Details
ubuntu-18.04 2.2
Details
ubuntu-18.04 2.3
Details
ubuntu-18.04 2.4
Details
ubuntu-18.04 2.5
Details
ubuntu-18.04 2.6
Details
ubuntu-18.04 2.7
Details
ubuntu-18.04 head
Details
ubuntu-18.04 jruby
Details
ubuntu-18.04 jruby-head ubuntu-18.04 jruby-head
Details
ubuntu-18.04 truffleruby-head
Details
macos-10.15 2.2
Details
macos-10.15 2.3
Details
macos-10.15 2.4
Details
macos-10.15 2.5
Details
macos-10.15 2.6
Details
macos-10.15 2.7
Details
macos-10.15 head
Details
macos-10.15 jruby
Details
macos-10.15 truffleruby-head macos-10.15 truffleruby-head
Details
windows-2019 2.2
Details
windows-2019 2.3
Details
windows-2019 2.4
Details
windows-2019 2.5
Details
windows-2019 2.6
Details
windows-2019 2.7
Details
windows-2019 mingw
Details
@wjordan wjordan mentioned this pull request Oct 1, 2020
7 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.