Better error handling during force shutdown #2271
Conversation
wjordan
force-pushed
the
force_shutdown_timing
branch
from
b0626f0
to
a290f14
Compare
lib/puma/client.rb
Outdated
| @@ -244,19 +244,17 @@ def eagerly_finish | |||
| end # IS_JRUBY | |||
|
|
|||
| def finish(timeout) | |||
| return true if @ready | |||
There was a problem hiding this comment.
Hm, what's the impact of changing the return value here? Any?
There was a problem hiding this comment.
Just looking through the rest of the code, seems like we never checked the value.
lib/puma/server.rb
Outdated
| # 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 |
There was a problem hiding this comment.
Since queue_requests is set at configure-time, I think we can move this outside the mutex, right?
lib/puma/server.rb
Outdated
| def queue_client(client, timeout) | ||
| if_queue_requests do | ||
| client.set_timeout timeout | ||
| @reactor.add client |
There was a problem hiding this comment.
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.
|
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. |
nateberkopec
added
waiting-for-changes
I agree, the Reactor refactoring in #2279 updates If that Reactor refactoring seems reasonable, this PR might be simpler following that one (e.g., 9975355.) |
|
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
force-pushed
the
force_shutdown_timing
branch
from
a290f14
to
fcfe784
Compare
nateberkopec
added
waiting-for-review
Description
Refactors tests covering shutdown behavior to be less flaky (the current tests depend on fragile
sleeptiming that isn't 100% deterministic), and also fixes a couple of subtle timing edge-cases around force-shutdown behavior:ForceShutdownto be raised in a thread during specific areas of the connection-processing cycle (marked bywith_force_shutdownblocks), to ensure that the raised error is always rescued and handled cleanly. (This fixes an issue where theforce_shutdown_after: 0option throws uncaught exceptions from the threadpool on shutdown)AddA fix for this issue was merged in Prevent connections from entering Reactor after shutdown begins #2377.@queue_mutexand helper methodsif_queue_requestsandqueue_clienttoServerto handle an edge case where the reactor shuts down while a thread is concurrently queuing a client. (Requests fail during phased restarts, hot restarts, and shutdown #2337)Your checklist for this pull request
[changelog skip]the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.