Can rack.after_reply be added to the spec?

[BlakeWilliams]

Both Puma and Unicorn have support for rack.after_reply which has been extremely useful but it's not part of the official Rack spec.

Given its usefulness and existing support, would it make sense to make this an official part of the spec to help encourage other rack based webservers to implement this functionality? If not, is there a path forward to making this an official part of the spec?

[HoneyryderChuck]

Why not https://github.com/rack/rack/blob/master/lib/rack/events.rb instead?

[BlakeWilliams]

Why not https://github.com/rack/rack/blob/master/lib/rack/events.rb instead?

I didn't get to dive too deeply into why, but the on_finish event was firing while the request/response was still open and was preventing the connection from being closed. I haven't tested other webservers but this was happening for at least unicorn.

[ioquatix]

(1) What is your use case for rack.after_reply?

(2) How does it work with HTTP/2?

[BlakeWilliams]

The use-case at the moment is for deferring non-critical work done in a request/response to happen after a user has already received the response. This primarily includes work that can't be a background job, like request-specific metrics that need to be sent over the wire and cache writes that we know won't need to be read back during the same request. The biggest benefit is that this lets us write abstractions that move potentially expensive work outside of the request so users see (often significantly) faster response times.

re: HTTP/2 I have to admit I'm not deeply familiar with http/2 and have only a basic understanding of it. I imagine it would be very similar to the existing implementations we have today. Some resource would be requested, we'd expose the ability to add lambdas to rack.after_reply and we'd call them once that resource has finished writing its response.

[HoneyryderChuck]

on_finish event was firing while the request/response was still open and was preventing the connection from being closed.

on_finish triggers after the response has been written to the client, which is the only level rack should work at. The connection shouldn't play a role here, as rack is for request/response management, that's what the servers are for. If this is what you need, then the callback being defined in the Webserver already makes sense, and shouldn't be ported here IMO.

[BlakeWilliams]

If this is what you need, then the callback being defined in the Webserver already makes sense, and shouldn't be ported here IMO.

Apologies, but I'm not sure I completely understand. I'm not proposing that Rack adds this functionality directly, but that Rack adds rack.after_reply to the spec as functionality that rack compliant web servers should implement (e.g. checked via Rack::Lint).

[HoneyryderChuck]

I understood, I'm just challenging why this should be specced. First, after_reply is too broad. What is a reply, a response? Then the events middleware should be what you need. You mention "after the connection closes". What is a connection? If it's a tcp socket, then it's a layer below what rack should control. Also, what if it doesn't close? That's the case for persistent connections or even HTTP/2.

IMO "rack.after_,reply" shouldn't be part of the spec. In fact, it already follows a known convention of prefixing keys with "rack.", a common practice for a multitude of known middleware which uses the rack env as a bucket for functionality and isn't part of the spec (such as warden and other several authentication Middlewares).

[ioquatix]

For your use case, e.g.

headers['rack.after_reply'] = proc {...code...}

Is it very different from:?

Thread.new{...code...}

or even

queue = Thread::Queue.new
thread = Thread.new do { queue.each(&:call) }

queue << proc {...code...}

It seems to me the only advantage would be the specific guarantee about when it happens but they would equally offload expensive processing.

Regarding rack.after_reply I think the name could be better, e.g. rack.after_response_flushed or rack.response_finished. We'd need to define some semantics for when it gets executed. Bearing in mind that not all servers are multi-threaded and therefore there is no such thing as "after the request is done" implying a good time to do extra work.

The next point is defining error handling semantics. What happens if the callback fails? What happens if the client bombs half way through writing the response body? What happens if it's a websocket request? Does this imply we'd want something like rack.response_failed or rack.client_disconnected? It seems tricky situation because we are potentially exposing the implementation details of the server or underlying protocol, which might not be a good direction.

[BlakeWilliams]

I understood, I'm just challenging why this should be specced. First, after_reply is too broad. What is a reply, a response? Then the events middleware should be what you need. You mention "after the connection closes". What is a connection? If it's a tcp socket, then it's a layer below what rack should control. Also, what if it doesn't close? That's the case for persistent connections or even HTTP/2.

Those are great questions. A large part of the value of adding it as a spec (to me) is that we can answer those questions to help define consistent behavior across implementations.

For your use case, e.g.

headers['rack.after_reply'] = proc {...code...}

Is it very different from:?

Thread.new{...code...}

or even

queue = Thread::Queue.new
thread = Thread.new do { queue.each(&:call) }

queue << proc {...code...}

It seems to me the only advantage would be the specific guarantee about when it happens but they would equally offload expensive processing.

Functionality wise they're not that different from one another. As you mentioned you lose some amount of predictability (i.e. running once per-request once the handler is complete) which is great to know and can help inform what is/isn't good to put in the queue. For example, one use-case is storing a set of data to be sent once the request is complete so that we can utilize batching functionality. Without rack.after_reply most of these calls were unable to utilize batching since they're scattered throughout the codebase and use the much slower non-batching behavior. Knowing that this will run at least once-per-request is helpful since it gives us some idea of how large that data-set may grow before we're able to flush it (and lets us avoid complexity around flushing while other code may still be appending data in another thread). The total time cost of that behavior went from roughly ~30-40ms per-request down to ~2ms per-request that ran after the user has already received a response.

Unfortunately in my situation the application isn't thread safe so we would need to be even more deliberate and cautious about what goes into the queue. I'm not sure if it's implied or not, but having the functionality coupled to a request is really nice too, since you can be confident that your code will be run if rack.after_reply is present.

Regarding rack.after_reply I think the name could be better, e.g. rack.after_response_flushed or rack.response_finished. We'd need to define some semantics for when it gets executed. Bearing in mind that not all servers are multi-threaded and therefore there is no such thing as "after the request is done" implying a good time to do extra work.

I'm 👍 on changing the name to make the behavior more clear. Like mentioned above, I think a large part of the value of adding this as a spec is that we can define those semantics and have them implemented consistently in the rack ecosystem. I'd be happy to help with that as much as I can.

The next point is defining error handling semantics. What happens if the callback fails? What happens if the client bombs half way through writing the response body? What happens if it's a websocket request? Does this imply we'd want something like rack.response_failed or rack.client_disconnected? It seems tricky situation because we are potentially exposing the implementation details of the server or underlying protocol, which might not be a good direction.

The existing error handling is currently a begin;rescue;end around calling each rack.after_reply which is admittedly a little too limiting. I think having each callback call wrapped in its own begin;rescue;end would be a nice change so that we can at least attempt to call each defined callback instead of failing fast. I haven't worked with websockets in Ruby in quite a while so I'm going to refrain from trying to provide any thoughts or answers until I've researched it a bit more and have a better understanding of the implications.

I agree with your statement about exposing too many implementation details being a less-than-great direction. I like rack.after_reply because it exposes the concept of (potentially overly reduced) "my server code is done running, now run this code" without impacting response timing. At least in my use-cases I would want the rack.after_reply callback code to run regardless of what happened in the request since the act of putting it into rack.after_reply implies that the code would have run anyways. The code should also still have access to request which would provide some level of being able to conditionally run.

[ioquatix]

@tenderlove I know you have some ideas around this, do you mind reviewing this request?

[tenderlove]

I agree we should add this and I like rack.response_finished. I'm not 100% sure what to do in the case of errors though. I think we should send this message even in the case an error occurs, but pass a value (maybe an exception object?). The thing is no webserver can really guarantee this thing will get executed (maybe the process segv's, or gets killd or something). I don't think that's a big deal, we just need to document it. Also each webserver should probably document its guarantees wrt executing this.

[ioquatix]

@BlakeWilliams do you want to have a go at cutting the spec for this? Update lib/rack/lint.rb

[nateberkopec]

Just linking for posterity - an excellent use of after_reply: https://github.com/testdouble/maybe_later

[BlakeWilliams]

@BlakeWilliams do you want to have a go at cutting the spec for this? Update lib/rack/lint.rb

Definitely, I'll make some time to work on that.

[BlakeWilliams]

Just opened up a draft PR that has a few open questions here: #1802

[ioquatix]

Merged in #1952