From 0089f7efc064c753d53ba975891f64e66090c281 Mon Sep 17 00:00:00 2001
From: Ewout <e@ewout.name>
Date: Fri, 2 Jul 2021 22:22:00 +0200
Subject: [PATCH] Fix performance of server-side SSL connection close.

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.
---
 lib/puma/minissl.rb | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/lib/puma/minissl.rb b/lib/puma/minissl.rb
index 64534872fb..77d57a5ba4 100644
--- a/lib/puma/minissl.rb
+++ b/lib/puma/minissl.rb
@@ -161,28 +161,13 @@ def flush
         @socket.flush
       end
 
-      def read_and_drop(timeout = 1)
-        return :timeout unless IO.select([@socket], nil, nil, 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