Project

General

Profile

Actions

Feature #10480

closed

Allow a catch-all approach to capture throws and rethrow them in a separate context

Added by rosenfeld (Rodrigo Rosenfeld Rosas) over 9 years ago. Updated over 9 years ago.

Status:
Closed
Target version:
[ruby-core:66103]

Description

Currently there's a bug in Devise/Rails when streaming is enabled through ActionController::Live module.

This module works by spawning a new thread to process the action. Devise uses a Rack middleware from Warden which works by a catch(:warden){} block. The problem is that when you ask Devise to authenticate in a before_action filter it will throw :warden when the authentication fails. Basically this is what happens:

catch(:warden) do
  thread = Thread.new {
    begin
      #...
      throw :warden
      #...
    rescue => e
      error = e
    end
  }
  thread.join
  raise error if error
end

This is just to illustrate. You can see the real code here:
https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/live.rb#L261

https://github.com/hassox/warden/blob/74162f2bf896b377472b6621ed1f6b40046525f4/lib/warden/manager.rb#L34

And the issues here:

https://github.com/plataformatec/devise/issues/2332
https://github.com/rails/rails/issues/13873

So, what happens is that throw is being called in a separate thread, outside the scope of the catch. Since it's not caught it raises an ArgumentError: "uncaught throw :warden".

There's currently no way to get the thrown symbol and the throw value from a generic handler as far as I know. Would it be possible to allow some kind of catch-all construction or at least to extract the throw params from the ArgumentError error? Or maybe use some new exception like UncaughtThrowException from which we would have access to the arguments?

Updated by nobu (Nobuyoshi Nakada) over 9 years ago

  • Description updated (diff)
  • Status changed from Open to Assigned

"Catch-all construction doesn't feel nice to me.
I think the new exception's name should be UncaughtThrowError, like others.

Updated by rosenfeld (Rodrigo Rosenfeld Rosas) over 9 years ago

That would be perfect as long as it includes the arguments passed to throw.

Updated by rosenfeld (Rodrigo Rosenfeld Rosas) over 9 years ago

I don't understand the code changes but looking at the tests this implementation looks pretty good to me. Could you please update the title for this issue so that it reflects the taken approach? Maybe "add an specialized UncaughtThrowError exception containing the throw arguments rather than raising a generic ArgumentError".

Updated by nobu (Nobuyoshi Nakada) over 9 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

Applied in changeset r48433.


vm_eval.c: UncaughtThrowError

  • vm_eval.c (rb_throw_obj): throw UncaughtThrowError instead of
    ArgumentError. [Feature #10480]
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0