Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for issue 463 #468

Closed
wants to merge 2 commits into from

3 participants

@brucek

Fixes issue 463 Make sure we call close on the result bodies for any results dispatched onward. This was causing rack locks to never be released when apps returned 404/405 in the middle of the cascade (and may have also leaked file handlers similar to issue 459).

Specifically, I was seeing these errors: (using Rails/rspec - to help future Googlers :-)

 ThreadError:
   thread 0x10251c270 tried to join itself
Bruce Krysiak added some commits
Bruce Krysiak Ignore .idea files a12231b
Bruce Krysiak Fixes issue 463 6a811e0
@oscardelben oscardelben commented on the diff
.gitignore
@@ -9,3 +9,4 @@ Gemfile.lock
.rbx
doc
/.bundle
+.idea

I would remove this from the PR, you can add it to your global gitignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@oscardelben oscardelben commented on the diff
lib/rack/mock.rb
@@ -73,7 +73,7 @@ def request(method="GET", uri="", opts={})
status, headers, body = app.call(env)
MockResponse.new(status, headers, body, errors)
ensure
- body.close if body.respond_to?(:close)
+ body.close if body.respond_to?(:close) unless opts[:dontclose]

Is this option here only for making the test work? I'm not sure when this option would be useful.

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

This approach is going to be misleading as it'll avoid middleware or app bugs only when using cascade, and not in other scenarios. I do not want to handle such bug reports.

@raggi raggi closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 14, 2012
  1. Ignore .idea files

    Bruce Krysiak authored
  2. Fixes issue 463

    Bruce Krysiak authored
This page is out of date. Refresh to see the latest.
View
1  .gitignore
@@ -9,3 +9,4 @@ Gemfile.lock
.rbx
doc
/.bundle
+.idea

I would remove this from the PR, you can add it to your global gitignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
5 lib/rack/cascade.rb
@@ -19,11 +19,12 @@ def initialize(apps, catch=[404, 405])
def call(env)
result = NotFound
- @apps.each do |app|
+ @apps[0..-2].each do |app|
result = app.call(env)
break unless @catch.include?(result[0].to_i)
+ result[2].close if result[2].respond_to?(:close)
end
-
+ result = @apps[-1].call(env) if @catch.include?(result[0].to_i) && @apps[-1]
result
end
View
2  lib/rack/mock.rb
@@ -73,7 +73,7 @@ def request(method="GET", uri="", opts={})
status, headers, body = app.call(env)
MockResponse.new(status, headers, body, errors)
ensure
- body.close if body.respond_to?(:close)
+ body.close if body.respond_to?(:close) unless opts[:dontclose]

Is this option here only for making the test work? I'm not sure when this option would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
# Return the Rack environment used for a request to +uri+.
View
18 test/spec_cascade.rb
@@ -28,6 +28,24 @@ def cascade(*args)
Rack::MockRequest.new(cascade).put("/foo").should.be.ok
end
+ should "call close on the response's body if the response was dispatch onward and it is not the last in the cascade" do
+ called = false
+ appa = Rack::URLMap.new("/nope" => lambda { |env|
+ [404, { "Content-Type" => "text/plain"}, Rack::BodyProxy.new([]) { called = true }]})
+ cascade = cascade([appa, app1])
+ Rack::MockRequest.new(cascade).get("/nope")
+ called.should.be.true
+ end
+
+ should "not call close if the last response in the cascade was dispatch onward" do
+ called = false
+ appa = Rack::URLMap.new("/nope" => lambda { |env|
+ [404, { "Content-Type" => "text/plain"}, Rack::BodyProxy.new([]) { called = true }]})
+ cascade = cascade([app1, appa])
+ Rack::MockRequest.new(cascade).get("/nope", :dontclose => true)
+ called.should.be.false
+ end
+
should "dispatch onward on whatever is passed" do
cascade = cascade([app1, app2, app3], [404, 403])
Rack::MockRequest.new(cascade).get("/cgi/../bla").should.be.not_found
Something went wrong with that request. Please try again.