Skip to content

Commit

Permalink
Insert rack-timeout before everything else in rails
Browse files Browse the repository at this point in the history
  • Loading branch information
kch committed Apr 15, 2013
1 parent 0ba8c02 commit 2888a5e
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/rack-timeout.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
when 2; Rails.configuration.middleware.use Rack::Timeout
when 3
class Rack::Timeout::Railtie < Rails::Railtie
initializer("rack-timeout.insert-rack-timeout") { |app| app.config.middleware.use Rack::Timeout }
initializer("rack-timeout.insert-rack-timeout") { |app| app.config.middleware.insert 0, Rack::Timeout }
end
end
end

9 comments on commit 2888a5e

@jjb
Copy link
Contributor

@jjb jjb commented on 2888a5e Apr 27, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kch this is nice, but there's a problem -- since it will be before airbrake/exceptional/etc, timeouts will be silently ignored by these systems.

i have a fork of rack-timeout where i built a feature to allow the user to specify a reporter lambda for timeouts. i actually haven't used it in production for a while and in general it could use some cleaning up. if this sounds interesting i can make a tidy pull request

jjb@ddcf82b

@kch
Copy link
Contributor Author

@kch kch commented on 2888a5e Apr 27, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm they actually get ignored? Since the timeout error is raised in the application thread, I'd think it would not matter.

@kch
Copy link
Contributor Author

@kch kch commented on 2888a5e Apr 27, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you might want to checkout the heroku branch.

This is most likely wherefrom a next release would be based off. It looks to me like your reporter could tie into set_state_and_log!.

@kch
Copy link
Contributor Author

@kch kch commented on 2888a5e Apr 27, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjb ^

@jjb
Copy link
Contributor

@jjb jjb commented on 2888a5e Apr 27, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kch okay, i'll check that out and also do some tests!

@kch
Copy link
Contributor Author

@kch kch commented on 2888a5e Apr 29, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I guess this would be a problem in your fork since you're using sane_timeout, which raises in the calling thread.

@jjb
Copy link
Contributor

@jjb jjb commented on 2888a5e May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kch did the heroku branch get merged into master? (the branch seems to be missing but i don't see the merge in the changelog)

@kch
Copy link
Contributor Author

@kch kch commented on 2888a5e May 16, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjb
Copy link
Contributor

@jjb jjb commented on 2888a5e May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, gotcha!

BTW, to be clear, I NEVER ran my branch in production. It was a side project I hacked on for a few days but didn't have time to get into good enough shape to deploy.

Please sign in to comment.