[PATCH] Add rack.after_reply functionality

[PATCH] Add rack.after_reply functionality

[Blake Williams]

This adds `rack.after_reply` functionality which allows rack middleware
to pass lambdas that will be executed after the client connection has
been closed.

This was driven by a need to perform actions in a request that shouldn't
block the request from completing but also don't make sense as
background jobs.

There is prior art of this being supported found in a few gems, as well
as this functionality existing in other rack based servers.
---
 lib/unicorn/http_server.rb |  4 ++++
 test/unit/test_server.rb   | 44 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index 05dad99..9889f55 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -629,6 +629,8 @@ def process_client(client)
       end
     end
 
+    env["rack.after_reply"] = []
+
     status, headers, body = @app.call(env)
 
     begin
@@ -651,6 +653,8 @@ def process_client(client)
     end
   rescue => e
     handle_error(client, e)
+  ensure
+    env["rack.after_reply"].each { |after_reply| after_reply.call }
   end
 
   def nuke_listeners!(readers)
diff --git a/test/unit/test_server.rb b/test/unit/test_server.rb
index 384fa6b..781750d 100644
--- a/test/unit/test_server.rb
+++ b/test/unit/test_server.rb
@@ -34,6 +34,24 @@ def call(env)
   end
 end
 
+class TestRackAfterReply
+  def initialize
+    @called = false
+  end
+
+  def call(env)
+    while env['rack.input'].read(4096)
+    end
+
+    env["rack.after_reply"] << -> { @called = true }
+
+    [200, { 'Content-Type' => 'text/plain' }, ["after_reply_called: #{@called}"]]
+  rescue Unicorn::ClientShutdown, Unicorn::HttpParserError => e
+    $stderr.syswrite("#{e.class}: #{e.message} #{e.backtrace.empty?}\n")
+    raise e
+  end
+end
+
 class WebServerTest < Test::Unit::TestCase
 
   def setup
@@ -114,6 +132,32 @@ def test_early_hints
     assert_match %r{^HTTP/1.[01] 200\b}, responses
   end
 
+  def test_after_reply
+    teardown
+
+    redirect_test_io do
+      @server = HttpServer.new(TestRackAfterReply.new,
+                               :listeners => [ "127.0.0.1:#@port"])
+      @server.start
+    end
+
+    sock = TCPSocket.new('127.0.0.1', @port)
+    sock.syswrite("GET / HTTP/1.0\r\n\r\n")
+
+    responses = sock.read(4096)
+    assert_match %r{\AHTTP/1.[01] 200\b}, responses
+    assert_match %r{^after_reply_called: false}, responses
+
+    sock = TCPSocket.new('127.0.0.1', @port)
+    sock.syswrite("GET / HTTP/1.0\r\n\r\n")
+
+    responses = sock.read(4096)
+    assert_match %r{\AHTTP/1.[01] 200\b}, responses
+    assert_match %r{^after_reply_called: true}, responses
+
+    sock.close
+  end
+
   def test_broken_app
     teardown
     app = lambda { |env| raise RuntimeError, "hello" }
-- 
2.29.2

Re: [PATCH] Add rack.after_reply functionality

[Eric Wong]

Blake Williams <blake@blakewilliams.me> wrote:
> This adds `rack.after_reply` functionality which allows rack middleware
> to pass lambdas that will be executed after the client connection has
> been closed.
> 
> This was driven by a need to perform actions in a request that shouldn't
> block the request from completing but also don't make sense as
> background jobs.

Thanks for the patch and explanation.

In the past, I've used response_body.close in middleware for
similar things, and nowadays Rack::BodyProxy exists in rack, too.

So I'm a little skeptical if this is actually needed, but it's
probably too late to do anything about:

> There is prior art of this being supported found in a few gems, as well
> as this functionality existing in other rack based servers.

Citation(s) needed.  Also, are there any proposal(s) to get this
into the Rack specification?

While I can't participate in discussions which requires a
GUI (JS/CAPTCHA) or accepting certain Terms-of-service,
the rack-devel@googlegroups.com mailing list still exists
and I can chime in there.

rack.early_hints could probably be in the Rack spec, too...

More below...

> diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
> index 05dad99..9889f55 100644
> --- a/lib/unicorn/http_server.rb
> +++ b/lib/unicorn/http_server.rb
> @@ -629,6 +629,8 @@ def process_client(client)
>        end
>      end
>  
> +    env["rack.after_reply"] = []
> +
>      status, headers, body = @app.call(env)
>  
>      begin
> @@ -651,6 +653,8 @@ def process_client(client)
>      end
>    rescue => e
>      handle_error(client, e)
> +  ensure
> +    env["rack.after_reply"].each { |after_reply| after_reply.call }
>    end
>  
>    def nuke_listeners!(readers)

I'll have to squash the following in, since an exception can be
raised if a client truncates the request:

diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index 9889f55c..c0f14ba5 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -654,7 +654,7 @@ def process_client(client)
   rescue => e
     handle_error(client, e)
   ensure
-    env["rack.after_reply"].each { |after_reply| after_reply.call }
+    env["rack.after_reply"].each(&:call) if env
   end
 
   def nuke_listeners!(readers)

Everything else seems OK.  Thanks.

Re: [PATCH] Add rack.after_reply functionality

[Blake Williams]

> In the past, I've used response_body.close in middleware for
> similar things, and nowadays Rack::BodyProxy exists in rack, too.
> 
> So I'm a little skeptical if this is actually needed, but it's
> probably too late to do anything about:

The first approach we took uses Rack::Events, which I believe is using
Rack::BodyProxy under the hood. We ran into an issue where all but
the last line of the response is written and the connection isn’t
actually closed yet when the callbacks are called.

> Citation(s) needed.  Also, are there any proposal(s) to get this
> into the Rack specification?

Not that I’m aware of. But I agree that it and early_hints could both be
good additions to the spec.

> I'll have to squash the following in, since an exception can be
> raised if a client truncates the request:

Good catch, sounds good to me.

> Everything else seems OK.  Thanks.

Awesome, thanks for the quick reply/feedback.

Re: [PATCH] Add rack.after_reply functionality

[Eric Wong]

Blake Williams <blake@blakewilliams.me> wrote:
> 
> > In the past, I've used response_body.close in middleware for
> > similar things, and nowadays Rack::BodyProxy exists in rack, too.
> > 
> > So I'm a little skeptical if this is actually needed, but it's
> > probably too late to do anything about:
> 
> The first approach we took uses Rack::Events, which I believe is using
> Rack::BodyProxy under the hood. We ran into an issue where all but
> the last line of the response is written and the connection isn’t
> actually closed yet when the callbacks are called.

OK.  Keep in mind this can be server-dependent since unicorn
can't support persistent connections while (most?) other servers
do (and they may disable TCP_CORK or Nagle, etc).

> > Citation(s) needed.  Also, are there any proposal(s) to get this
> > into the Rack specification?
> 
> Not that I’m aware of. But I agree that it and early_hints could both be
> good additions to the spec.

By "Citation(s) needed", can you list some other (preferably
well-known) Rack servers which also implement this feature?

We wouldn't want current unicorn users thinking we're adopting
random new things which nobody else supports :>

Thanks.

Re: [PATCH] Add rack.after_reply functionality

[Blake Williams]

> OK.  Keep in mind this can be server-dependent since unicorn
> can't support persistent connections while (most?) other servers
> do (and they may disable TCP_CORK or Nagle, etc).

I see, I’ll look into this as well.

>>> Citation(s) needed.  Also, are there any proposal(s) to get this
>>> into the Rack specification?
>> 
>> Not that I’m aware of. But I agree that it and early_hints could both be
>> good additions to the spec.
> 
> By "Citation(s) needed", can you list some other (preferably
> well-known) Rack servers which also implement this feature?
> 
> We wouldn't want current unicorn users thinking we're adopting
> random new things which nobody else supports :>
> 
> Thanks.


Totally understood. I looked at Puma, Thin, and Webrick and only Puma
supports this functionality without a third-party gem. There is at least one
third party gem adding this functionality to web servers that don’t support it,
but it doesn’t look like it’s being maintained any longer.

I briefly dove into the Puma source and it looks like rack.after_reply has
been supported since 2011. It’s also still in use by Puma for their common 
logger middleware patch.

Re: [PATCH] Add rack.after_reply functionality

[Eric Wong]

Blake Williams <blake@blakewilliams.me> wrote:
> Totally understood. I looked at Puma, Thin, and Webrick and only Puma
> supports this functionality without a third-party gem. There is at least one
> third party gem adding this functionality to web servers that don’t support it,
> but it doesn’t look like it’s being maintained any longer.

Alright, I guess that third-party gem is no longer relevant.

> I briefly dove into the Puma source and it looks like rack.after_reply has
> been supported since 2011. It’s also still in use by Puma for their common 
> logger middleware patch.

Thanks, pushed to https://yhbt.net/unicorn.git as
commit 673c15e3f020bccc0336838617875b26c9a45f4e
with Puma noted (see below) and extra `env' check added.

Anything else?  Will wait a few days/week for others to chime in
and probably cut 5.8 in a week or so.

Btw, a sidenote about some strangeness in your mail replies:

  I'm not sure why, but your "Reply-To" header is set to the
  unbracketed Message-ID ("20201209094344.GA25593@dcvr") of the
  message, which makes no sense...

  I wonder if you or your mail client is confusing "Reply-To:"
  with the "In-Reply-To:" header.  They're completely different:
  "Reply-To:" should be an email address you control (not a
  Message-ID) if it differs from what's in the "From:" header.

  In any case, your mail client already sets the "References:"
  header correctly (probably w/o any interaction on your part),
  and the "In-Reply-To:" header is not necessary (and it needs
  angle brackets, anyways, since comments are allowed).

  cf. https://www.jwz.org/doc/threading.html


Author: Blake Williams <blake@blakewilliams.me>
Date:   Tue Dec 8 16:47:16 2020 -0500

    Add rack.after_reply functionality
    
    This adds `rack.after_reply` functionality which allows rack middleware
    to pass lambdas that will be executed after the client connection has
    been closed.
    
    This was driven by a need to perform actions in a request that shouldn't
    block the request from completing but also don't make sense as
    background jobs.
    
    There is prior art of this being supported found in a few gems, as well
    as this functionality existing in other rack based servers (e.g. Puma).
    
    [ew: check if `env' is set in ensure statement]
    
    Acked-by: Eric Wong <e@80x24.org>

Re: [PATCH] Add rack.after_reply functionality

[Blake Williams]

> Thanks, pushed to https://yhbt.net/unicorn.git as
> commit 673c15e3f020bccc0336838617875b26c9a45f4e
> with Puma noted (see below) and extra `env' check added.
> 
> Anything else?  Will wait a few days/week for others to chime in
> and probably cut 5.8 in a week or so.

Thanks, that should be everything on my end. Looking forward to the
release.

> Btw, a sidenote about some strangeness in your mail replies:
> 
>  I'm not sure why, but your "Reply-To" header is set to the
>  unbracketed Message-ID ("20201209094344.GA25593@dcvr") of the
>  message, which makes no sense...
> 
>  I wonder if you or your mail client is confusing "Reply-To:"
>  with the "In-Reply-To:" header.  They're completely different:
>  "Reply-To:" should be an email address you control (not a
>  Message-ID) if it differs from what's in the "From:" header.
> 
>  In any case, your mail client already sets the "References:"
>  header correctly (probably w/o any interaction on your part),
>  and the "In-Reply-To:" header is not necessary (and it needs
>  angle brackets, anyways, since comments are allowed).
> 
>  cf. https://www.jwz.org/doc/threading.html

Strange, thanks for the heads up. I’ve updated the settings, so
hopefully that issue is resolved.