Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Close the original body passed to Rack::ETag #607

Closed
wants to merge 3 commits into from

4 participants

@davidcelis

The Rack spec states:

If the Body responds to close, it will be called after iteration. If
the body is replaced by a middleware after action, the original body
must be closed first, if it responds to close.

After Rack::ETag replaces the response body, close the original body.

Backstory:

@benweint and I found this bug while debugging a problem with the
ActiveRecord::ConnectionAdapters::ConnectionManagement middleware
when it was included deeper in the stack than Rack::ETag. As the
response trickles back up the stack, ConnectionManagement replaces
the body with a BodyProxy which, when closed, will close database
connections. However, ETag then replaces the BodyProxy without
closing it, so our database connections were never being closed.

Signed-off-by: David Celis me@davidcel.is

@davidcelis davidcelis Close the original body passed to Rack::ETag
The Rack spec states:

> If the Body responds to `close`, it will be called after iteration. If
> the body is replaced by a middleware after action, the original body
> must be closed first, if it responds to close.

After Rack::ETag replaces the response body, close the original body.

Signed-off-by: David Celis <me@davidcel.is>
3a388bc
@davidcelis

Any updates for me here? This is kind of a lame bug.

@davidcelis

Couple more commits added; namely, it seems that Rack::ConditionalGet also disobeys Rack's spec and doesn't close the original body that it receives. We've fixed that as well.

@raggi raggi commented on the diff
test/spec_conditionalget.rb
@@ -2,6 +2,8 @@
require 'rack/conditionalget'
require 'rack/mock'
+require File.expand_path('../closable_body', __FILE__)
@raggi Owner
raggi added a note

test is on the load path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi raggi commented on the diff
test/spec_etag.rb
@@ -3,15 +3,17 @@
require 'rack/mock'
require 'time'
+require File.expand_path('../closable_body', __FILE__)
@raggi Owner
raggi added a note

test is on the load path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi
Owner

Correction (edit): The body is now sometimes closed when the server will still attempt to render it.

Body should only be closed by the middleware if it is not handed to the webserver. See SPEC for more details.

@benweint

@raggi Thanks! Could you clarify the situation in which the body would be closed and the server would still attempt to render it and/or point us towards the specific part of the SPEC you're referring to?

The intent of this change was that the body would only be closed if it were being replaced by the middlewares in question, per this bit of the SPEC:

If the Body responds to +close+, it will be called after iteration. If
the body is replaced by a middleware after action, the original body
must be closed first, if it responds to close.
@tenderlove
Owner

@benweint the webserver will call close on the body, so you can't just call close on the body inside the call method. You have to wait until the webserver calls close on the body. This means that the ETag middleware needs to wrap it's new body with a BodyProxy that will call close on the original body. Something like this:

diff --git a/lib/rack/etag.rb b/lib/rack/etag.rb
index 5fa09ab..5f4ddd5 100644
--- a/lib/rack/etag.rb
+++ b/lib/rack/etag.rb
@@ -23,7 +23,11 @@ module Rack
       status, headers, body = @app.call(env)

       if etag_status?(status) && etag_body?(body) && !skip_caching?(headers)
+        original_body = body
         digest, body = digest_body(body)
+        body = Rack::BodyProxy.new(body) do
+          original_body.close
+        end
         headers['ETag'] = %("#{digest}") if digest
       end
@benweint

Thanks @tenderlove!

I'm sure I'm just being dense here, but the thing I'm missing is this: if the body is being replaced by the middleware, how will the webserver even have a reference to the original body in order to call close on it?

If you're saying that it is inappropriate for a middleware to call close on the body even when replacing it, and the BodyProxy approach must be used in order to get correct behavior, then I'd argue the spec should be updated to make this more clear (the current language as quoted above suggests to me that the correct behavior is for close to be called on the body at the time of replacement).

@tenderlove
Owner

@benweint no problem. With my patch, the webserver will have a reference to the newly created BodyProxy, and that BodyProxy will have a reference to the original body (via the closure). It is appropriate for the middleware to call close on the body, but it has to do it at the right time; namely when the webserver calls close on the new body.

I'm not sure how to update the spec to make that more clear. Can you can help us make it more clear?

Don't be afraid to ask if you have more questions! :D

@benweint

@tenderlove thanks for the explanation!

I think my question was unclear: I understand the mechanics of BodyProxy, but not why it's necessary in this case. Specifically: what is the potential fallout if a middleware replaces the body and calls close on the original body before the response makes it all the way out to the web server? James' comment suggested that the issue might lie with the the webserver receiving a reference to an already-closed body, and my question was around how this would happen with the proposed patch.

Can you sketch out an example of what might go wrong if a middleware were to replace the body and call close on the original body to help me understand?

With regards to the spec, how about replacing the two sentences quoted above with something like this:

If the Body responds to +close+, it will be called after iteration. If
the body is replaced by a middleware after action, the replacement body
must proxy calls to +close+ to the original body, if the original body responds
to +close+.
@tenderlove
Owner
@tenderlove
Owner
@benweint

I'm not sure I grok the infinite stream case (in the case where the body is an infinite stream, the whole idea of a middleware 'replacing' the body doesn't really seem to apply), but the Rails case makes some sense (though it seems like it would only apply if one had a middleware that was accessing those global resources).

I'll rework this with @davidcelis to use BodyProxy.

Any thoughts on the proposed SPEC modification?

@tenderlove
Owner

in the case where the body is an infinite stream, the whole idea of a middleware 'replacing' the body doesn't really seem to apply

The replaced body could simply mean that the middleware does some sort of translation on the stream.

though it seems like it would only apply if one had a middleware that was accessing those global resources

Some people log to a database after the request is completed. Also, some erb templates access the database, but don't do it until each is called on the body (though it wouldn't matter in the ETag case, since ETags need the whole body to compute the hash).

Any thoughts on the proposed SPEC modification?

I think your wording is much more clear. I don't know about the ceremony behind The Changing Of The Spec, so I'll ping @raggi about what we need to do to change it.

@raggi raggi commented on the diff
test/closable_body.rb
@@ -0,0 +1,18 @@
+class ClosableBody
@raggi Owner
raggi added a note

You can just use StringIO here, like other tests do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi
Owner

Closed by 4d9e1b2.

Another case: in rails, we use the close method to determine when all data has been written to the client. Then we know it's ok to release resources (such as database connections). Calling close at the wrong time could cause these "global" resources to be released when the response actually hasn't finished

While everything you say is true, I am a little concerned by the part "to determine when all data has been written to the client". This isn't really possible in the Rack SPEC today, and while this proxy may work for some cases, there are other precedents in the Rack code base for this to be false:


lib/rack/cascade.rb: last_body.close if last_body.respond_to? :close
lib/rack/chunked.rb: @body.close if @body.respond_to?(:close)
lib/rack/content_length.rb: obody.close if obody.respond_to?(:close)
lib/rack/deflater.rb: body.close if body.respond_to?(:close)
lib/rack/deflater.rb: @body.close if @body.respond_to?(:close)
lib/rack/deflater.rb: @body.close if @body.respond_to?(:close)

In general, if the entire headers have been shipped up to middleware, and the body has been consumed by middleware, then that middleware is "the client" as far as the application is concerned.

If there's a case where middleware higher in the return chain is planning to use resources that are closed by #close, then I would say that middleware is at fault. In that case I would advise, for middleware that share state up and down the chain, that the free contract be as follows:

Shared resources should be freed when both of the following conditions are true:

  • All middleware with a reference to the state have completed processing
  • The body stream (if a stream) has been closed

I would like this to be better, but for now it's what we have. HTH.

@raggi raggi closed this
@raggi
Owner

Wait, I'll correct myself here. You're right, we need to use BodyProxy everywhere for Lock to work.

We need to audit the existing code base to ensure all cases of this have been fixed. There will be races!

@raggi
Owner

Fixed other conditions.

@davidcelis

Thanks, @raggi. I would have liked to have helped with the actual fixes, but oh well! Thanks for taking the time. :+1:

@raggi
Owner

I understand - was only down to time that I took this approach - thanks for your patches!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 29, 2013
  1. @davidcelis

    Close the original body passed to Rack::ETag

    davidcelis authored
    The Rack spec states:
    
    > If the Body responds to `close`, it will be called after iteration. If
    > the body is replaced by a middleware after action, the original body
    > must be closed first, if it responds to close.
    
    After Rack::ETag replaces the response body, close the original body.
    
    Signed-off-by: David Celis <me@davidcel.is>
Commits on Nov 18, 2013
  1. @benweint @davidcelis
  2. @benweint @davidcelis

    Close body passed to the ConditionalGet middleware

    benweint authored davidcelis committed
This page is out of date. Refresh to see the latest.
View
1  lib/rack/conditionalget.rb
@@ -28,6 +28,7 @@ def call(env)
status = 304
headers.delete('Content-Type')
headers.delete('Content-Length')
+ body.close if body.respond_to?(:close)
body = []
end
[status, headers, body]
View
2  lib/rack/etag.rb
@@ -62,6 +62,8 @@ def digest_body(body)
(digest ||= Digest::MD5.new) << part unless part.empty?
end
+ body.close if body.respond_to?(:close)
+
[digest && digest.hexdigest, parts]
end
end
View
18 test/closable_body.rb
@@ -0,0 +1,18 @@
+class ClosableBody
@raggi Owner
raggi added a note

You can just use StringIO here, like other tests do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ def initialize(content)
+ @content = content
+ @closed = false
+ end
+
+ def each
+ yield @content
+ end
+
+ def close
+ @closed = true
+ end
+
+ def closed?
+ @closed
+ end
+end
View
14 test/spec_conditionalget.rb
@@ -2,6 +2,8 @@
require 'rack/conditionalget'
require 'rack/mock'
+require File.expand_path('../closable_body', __FILE__)
@raggi Owner
raggi added a note

test is on the load path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
describe Rack::ConditionalGet do
def conditional_get(app)
Rack::Lint.new Rack::ConditionalGet.new(app)
@@ -99,4 +101,16 @@ def conditional_get(app)
response.body.should.equal 'TEST'
end
+ should "close the body when replacing it if the body responds to close" do
+ body = ClosableBody.new('test')
+
+ app = conditional_get(lambda { |env|
+ [200, {'Etag' => '1234', 'Content-Type' => 'text/plain'}, body] })
+
+ response = Rack::MockRequest.new(app).
+ get("/", 'HTTP_IF_NONE_MATCH' => '1234')
+
+ body.should.be.closed
+ end
+
end
View
13 test/spec_etag.rb
@@ -3,15 +3,17 @@
require 'rack/mock'
require 'time'
+require File.expand_path('../closable_body', __FILE__)
@raggi Owner
raggi added a note

test is on the load path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
describe Rack::ETag do
def etag(app, *args)
Rack::Lint.new Rack::ETag.new(app, *args)
end
-
+
def request
Rack::MockRequest.env_for
end
-
+
def sendfile_body
res = ['Hello World']
def res.to_path ; "/tmp/hello.txt" ; end
@@ -95,4 +97,11 @@ def res.to_path ; "/tmp/hello.txt" ; end
response = etag(app).call(request)
response[1]['ETag'].should.be.nil
end
+
+ should "close the original body if it responds to close" do
+ body = ClosableBody.new('hello')
+ app = lambda { |env| [200, {'Content-Type' => 'text/plain'}, body] }
+ etag(app).call(request)
+ body.should.be.closed
+ end
end
Something went wrong with that request. Please try again.