1
0
Fork 0
mirror of https://github.com/puma/puma.git synced 2022-11-09 13:48:40 -05:00

[close #1802] Close listeners on SIGTERM

Currently when a SIGTERM is sent to a puma cluster, the signal is trapped, then sent to all children, it then waits for children to exit and then the parent process exits. The socket that accepts connections is only closed when the parent process calls `exit 0`. The problem with this flow is there is a period of time where there are no child processes to work on an incoming connection, however the socket is still open so clients can connect to it. When this happens, the client will connect, but the connection will be closed with no response. Instead, the desired behavior is for the connection from the client to be rejected. This allows the client to re-connect, or if there is a load balance between the client and the puma server, it allows the request to be routed to another node.

This PR fixes the existing behavior by manually closing the socket when SIGTERM is received before shutting down the workers/children processes. When the socket is closed, any incoming requests will fail to connect and they will be rejected, this is our desired behavior. Existing requests that are in-flight can still respond.

 ## Test


This behavior is quite difficult to test, you'll notice that the test is far longer than the code change. In this test we send an initial request to an endpoint that sleeps for 1 second. We then signal to other threads that they can continue. We send the parent process a SIGTERM, while simultaneously sending other requests. Some of these will happen after the SIGTERM is received by the server. When that happens we want none of the requests to get a `ECONNRESET` error, this would indicate the request was accepted but then closed. Instead we want `ECONNREFUSED`.

I ran this test in a loop for a few hours and it passes with my patch, it fails immediately if you remove the call to close the listeners.

```
$ while m test/test_integration.rb:235; do :; done
```

 ## Considerations

This PR only fixes the problem for "cluster" (i.e. multi-worker) mode. When trying to reproduce the test with single mode, on (removing the `-w 2` config) it already passes. This leads us to believe that either the bug does not exist in single threaded mode, or at the very least reproducing the bug via a test in the single threaded mode requires a different approach.

Co-authored-by: Danny Fallon <Danny.fallon.ie+github@gmail.com>
Co-authored-by:  Richard Schneeman <richard.schneeman+foo@gmail.com>
This commit is contained in:
Richard Schneeman 2019-05-30 13:09:17 -05:00 committed by schneems
parent d39fe92a2d
commit 184e1510a9
3 changed files with 58 additions and 0 deletions

View file

@ -401,6 +401,8 @@ module Puma
log "Early termination of worker"
exit! 0
else
@launcher.send(:close_binder_listeners)
stop_workers
stop

4
test/rackup/1second.ru Normal file
View file

@ -0,0 +1,4 @@
run lambda { |env|
sleep 1
[200, {}, ["Hello World"]]
}

View file

@ -232,6 +232,58 @@ class TestIntegration < Minitest::Test
assert_equal "Hello World", new_reply
end
def test_sigterm_closes_listeners_on_forked_servers
skip NO_FORK_MSG unless HAS_FORK
pid = start_forked_server("-w 2 -q test/rackup/1second.ru")
threads = []
initial_reply = nil
next_replies = []
condition_variable = ConditionVariable.new
mutex = Mutex.new
threads << Thread.new do
s = connect
mutex.synchronize { condition_variable.broadcast }
initial_reply = read_body(s)
end
threads << Thread.new do
mutex.synchronize {
condition_variable.wait(mutex, 1)
Process.kill("SIGTERM", pid)
}
end
10.times.each do |i|
threads << Thread.new do
mutex.synchronize { condition_variable.wait(mutex, 1.5) }
begin
s = connect
read_body(s)
next_replies << :success
rescue Errno::ECONNRESET
# connection was accepted but then closed
# client would see an empty response
next_replies << :connection_reset
rescue Errno::ECONNREFUSED
# connection was was never accepted
# it can therefore be re-tried before the
# client receives an empty reponse
next_replies << :connection_refused
end
end
end
threads.map(&:join)
assert_equal "Hello World", initial_reply
assert_includes next_replies, :connection_refused
refute_includes next_replies, :connection_reset
end
# It does not share environments between multiple generations, which would break Dotenv
def test_restart_restores_environment
# jruby has a bug where setting `nil` into the ENV or `delete` do not change the