Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed Rack::Lock so it correctly releases mutexes on throw #425

Merged
merged 1 commit into from

3 participants

@SamSaffron
  • amend it so Rack::Lock releases the mutex on throws as well and raises
  • added throw test
  • fixed raise test
@SamSaffron SamSaffron - correct existing raise test
- amend it so rack lock releases the mutex on throws as well and raises
- added raise test
5c230ff
@travisbot

This pull request passes (merged 5c230ff into d084a14).

@SamSaffron

Note, @rkh @raggi, besides being more correct, this code does enable throw :async if Rack::Lock is in the stack.

I know what you are thinking, WTF, why would you have both Rack::Lock and use throw :async, the reason is that Rails is a bit crazy like that.

The config.threadsafe! thing that is going to be default in Rails 4 is kind of patchy on Rails 3, in particular bundling assets and running spork go totally nuts. Now, if you are going to be serving sockets or long polling from your rails app and want the middleware to live further up the stack Rack::Lock clashes.

I like having the middleware that does the "setup" for the long polling (figure out current user - and call db for a few things to be further up), once I am done with the "setup" I throw :async and free the server to process the next request. If I really need to lock up rack later on when I am servicing stuff I can pass my own mutex in to Rack lock and lock on it.

The body proxy stuff is a bit weird imho, but I have not touched that. For example if Rack::Lock is not first in the stack or you get an exception prior to servicing the whole request (timeout or some other weird thing) you deadlock the next request.

@rkh
Owner
rkh commented

This seems fine to me. Is there any chance you could add some sort of test?

@rkh
Owner
rkh commented

The body proxy stuff is a bit weird imho, but I have not touched that. For example if Rack::Lock is not first in the stack or you get an exception prior to servicing the whole request (timeout or some other weird thing) you deadlock the next request

Really? Only if you have middleware that doesn't implement the Rack protocol properly, no?

@SamSaffron

@rkh I both added a test and fixed a faulty test SamSaffron@5c230ff#L1R143

@SamSaffron

Really? Only if you have middleware that doesn't implement the Rack protocol properly, no?

You know, its all semicolons, all I am going on here is feelings, it feels like the scope is odd, but you can argue that .each must be drained for all rack consumers, totally get that.

@rkh
Owner
rkh commented

You know, its all semicolons, all I am going on here is feelings, it feels like the scope is odd, but you can argue that .each must be drained for all rack consumers, totally get that.

How do you mean? This is not just semicolons. If a middleware hands on a new body and does not call close on the old one, it's violating Rack protocol in a really bad way. It means that, besides running into deadlocks and stuff, apps using that middleware will probably sooner or later run out of file handlers.

@rkh rkh merged commit ea7ed1a into rack:master
@SamSaffron

@rkh

How do you mean?

I mean that this middleware extends the scope of the mutex beyond code it can control, it feels odd to me, it kind of feels like these kind of mutexes belong in the web server outside of the rack stack when code can ensure they are released. But it is nothing really to stress about, I follow why stuff is done the way it is.

@rkh
Owner
rkh commented

Yeah, the current situation is suboptimal, but IMHO, that's due to shortcomings in the Rack protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 5, 2012
  1. @SamSaffron

    - correct existing raise test

    SamSaffron authored
    - amend it so rack lock releases the mutex on throws as well and raises
    - added raise test
This page is out of date. Refresh to see the latest.
Showing with 12 additions and 5 deletions.
  1. +3 −4 lib/rack/lock.rb
  2. +9 −1 test/spec_lock.rb
View
7 lib/rack/lock.rb
@@ -13,12 +13,11 @@ def call(env)
old, env[FLAG] = env[FLAG], false
@mutex.lock
response = @app.call(env)
- response[2] = BodyProxy.new(response[2]) { @mutex.unlock }
+ body = BodyProxy.new(response[2]) { @mutex.unlock }
+ response[2] = body
response
- rescue Exception
- @mutex.unlock
- raise
ensure
+ @mutex.unlock unless body
env[FLAG] = old
end
end
View
10 test/spec_lock.rb
@@ -134,11 +134,19 @@ def close; @close_called = true; end
should "unlock if the app raises" do
lock = Lock.new
env = Rack::MockRequest.env_for("/")
- app = lock_app(lambda { raise Exception })
+ app = lock_app(lambda { raise Exception }, lock)
lambda { app.call(env) }.should.raise(Exception)
lock.synchronized.should.equal false
end
+ should "unlock if the app throws" do
+ lock = Lock.new
+ env = Rack::MockRequest.env_for("/")
+ app = lock_app(lambda {|env| throw :bacon }, lock)
+ lambda { app.call(env) }.should.throw(:bacon)
+ lock.synchronized.should.equal false
+ end
+
should "set multithread flag to false" do
app = lock_app(lambda { |env|
env['rack.multithread'].should.equal false
Something went wrong with that request. Please try again.