Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix BodyProxy#close so it works correctly when close invoked more than one time #191

Closed
wants to merge 1 commit into from

2 participants

@janx

In Rack::Lock the BodyProxy is initialized with a block { @mutex.unlock }, if this proxy closes many times, @mutex.unlock will be called multiple times and cause an exception.

For example, rack-test will try to close the proxy a second time (https://github.com/brynary/rack-test/blob/master/lib/rack/mock_session.rb#L33), raise "ThreadError: Attempt to unlock a mutex which is not locked", and break lots of my tests :-(

@rkh
Owner

I think the issue was introduced by f0a1e6f, but we cannot simply revert that, since the previous behavior was also wrong. I already thought about a solution and will wrap up a commit and tests as soon as possible. I will merge your commit as soon as I look into this (tonight or tomorrow). Keep in mind that the same issue will occur with files and other objects that may only be closed once.

@rkh rkh was assigned
@rkh rkh referenced this pull request from a commit
@rkh rkh Have MockRequest call close on the body rather than MockResponse.
That way close is called automatically when testing just with
vanilla Rack, but not called twice when using other testing libs
like rack-test.

Related to #191.
4485088
@rkh rkh referenced this pull request from a commit
@rkh rkh let Rack::BodyProxy raise an IOError (like IO and StringIO do) when c…
…alling #close twice. Related to #191.
c02443d
@rkh
Owner

The real issue was, that rack-test was calling close again. Fixed it by not calling close in MockResponse. Still merged your patch and modified it to make it behave like IO (raise an IOError) to not let this go unnoticed but still play nice with some folks exception handling. Thanks for looking into this and running your tests against master, @janx.

@rkh rkh closed this
@janx

Thanks, it looks nice. However I think there's still a small bug: you removed the begin..end block from #close, which means @block.call will always run even if an IOError is raised; and if @block.call raise an exception, that new exception will overwrite IOError.

I don't know how to add commit to this thread .. see: janx@cb703b6

@rkh
Owner

Merged in that commit, thanks.

@janx

That's fast, thanks :)

@rkh rkh referenced this pull request from a commit
@rkh rkh let Rack::BodyProxy raise an IOError (like IO and StringIO do) when c…
…alling #close twice. Related to #191.
00503be
@rkh rkh referenced this pull request from a commit
@rkh rkh Have MockRequest call close on the body rather than MockResponse.
That way close is called automatically when testing just with
vanilla Rack, but not called twice when using other testing libs
like rack-test.

Related to #191.
9cc14bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 13, 2011
  1. @janx

    fix BodyProxy#close

    janx authored
This page is out of date. Refresh to see the latest.
Showing with 21 additions and 4 deletions.
  1. +12 −4 lib/rack/body_proxy.rb
  2. +9 −0 test/spec_body_proxy.rb
View
16 lib/rack/body_proxy.rb
@@ -1,7 +1,7 @@
module Rack
class BodyProxy
def initialize(body, &block)
- @body, @block = body, block
+ @body, @block, @closed = body, block, false
end
def respond_to?(*args)
@@ -9,9 +9,17 @@ def respond_to?(*args)
end
def close
- @body.close if @body.respond_to? :close
- ensure
- @block.call
+ return if closed?
+ begin
+ @body.close if @body.respond_to? :close
+ ensure
+ @block.call
+ @closed = true
+ end
+ end
+
+ def closed?
+ @closed
end
def method_missing(*args, &block)
View
9 test/spec_body_proxy.rb
@@ -0,0 +1,9 @@
+describe Rack::BodyProxy do
+ should 'not close more than one time' do
+ count = 0
+ proxy = Rack::BodyProxy.new([]) { count += 1 }
+ proxy.close
+ proxy.close
+ count.should == 1
+ end
+end
Something went wrong with that request. Please try again.