Skip to content

Commit

Permalink
Fix performance of server-side SSL connection close. (#2675)
Browse files Browse the repository at this point in the history
When the server wants to close a persistent SSL connection
because it was idle for `persistent_timeout`, the call stack is:

    Reactor.wakeup!
      Server#reactor_wakeup
        Client#close
          MiniSSL::Socket#close

The close method is called from within the reactor thread.

In this case, `should_drop_bytes?` is true, because `@engine.shutdown`
is false the first time it is called.

Then, `read_and_drop(1)` is called, which starts by selecting on the
socket. Now because this is a server-initiated close of an idle
connection, in almost all cases there will be nothing to select,
and hence the thread will just wait for 1 second.

Since this is called by the reactor, the reactor will halt for 1 second
and not be able to buffer any data during that time, which has huge
effects on overall worker throughput.

Now I'm not sure what is the use to read from the socket?

* From the docs:

    It is acceptable for an application to only send its shutdown alert and
    then close the underlying connection without waiting for the peer's
    response.
    https://www.openssl.org/docs/man1.1.1/man3/SSL_shutdown.html

* The existing code did not seem to send any shutdown alert,
  because the shutdown method was called only on the engine, but the
  engine is not connected to the actual TCP socket. The resulting data
  still needed to be compied

* If the server wants to wait for the client's close_notify shutdown alert,
  then this waiting needs to happen with a non-blocking select in the reactor,
  so other work can be done in the meantime. This is not trivial to
  implement, though.

Note that when the client initiated the close and the data was already
read into the engine, @engine.shutdown will return true immediately,
hence this is only a problem when the server wants to close a
connection.
  • Loading branch information
devwout committed Sep 8, 2021
1 parent a0cab5d commit 319f84d
Showing 1 changed file with 5 additions and 20 deletions.
25 changes: 5 additions & 20 deletions lib/puma/minissl.rb
Expand Up @@ -161,28 +161,13 @@ def flush
@socket.flush
end

def read_and_drop(timeout = 1)
return :timeout unless @socket.wait_readable(timeout)
case @socket.read_nonblock(1024, exception: false)
when nil
:eof
when :wait_readable
:eagain
else
:drop
end
end

def should_drop_bytes?
@engine.init? || !@engine.shutdown
end

def close
begin
# Read any drop any partially initialized sockets and any received bytes during shutdown.
# Don't let this socket hold this loop forever.
# If it can't send more packets within 1s, then give up.
return if [:timeout, :eof].include?(read_and_drop(1)) while should_drop_bytes?
unless @engine.shutdown
while alert_data = @engine.extract
@socket.write alert_data
end
end
rescue IOError, SystemCallError
Thread.current.purge_interrupt_queue if Thread.current.respond_to? :purge_interrupt_queue
# nothing
Expand Down

0 comments on commit 319f84d

Please sign in to comment.