Right now, filters run for as many times as you call, eg.,
before_filter.
If you do something like:
5.times { before_filter :foo }
it might be expect, but I think the more common case of:
@ ApplicationController
before_filter :foo
@ SubController
before_filter :foo, :except => :bar
yields the unexpected result where :foo will run twice. (Luckly, for
sub/bar it doesn''t run at all, which is what I would expect)
I understand that in this particular case using skip_before_filter
would fix it, but...
Do you really think it''s a good idea to run filters more than once?
How about maybe adding a uniq option to pre/append_filter_to_chain,
that would ensure a filter is never added twice (but related
conditions may be changed)?
It would seem to me most filters that end up running twice or thrice
do so harmlessly, but needlessly. At most, hindering performance.
On Jul 31, 2006, at 11:17 AM, Caio Chassot wrote:> Do you really think it''s a good idea to run filters more than once? > > How about maybe adding a uniq option to pre/append_filter_to_chain, > that would ensure a filter is never added twice (but related > conditions may be changed)? > > It would seem to me most filters that end up running twice or > thrice do so harmlessly, but needlessly. At most, hindering > performance.I think you''re right that most filters should only run once. But I worry that doing this might cause problems in border cases. What about putting warnings in the logs when filters are run more than once rather than prohibiting it all together. Or maybe throw an exception, but allow that to be suppressed by providing something like: before_filter :foo, :rerun => true Perhaps :rerun isn''t the best name, but I think you get the idea. You could throw the exception in before_filter using .include? and avoid the overhead of keeping a call record or constantly making the list unique. -Mat PS., I ran a quick benchmark for uniq vs. include? and found this: Rehearsal ----------------------------------------------- uniq 1.430000 0.070000 1.500000 ( 1.531704) if include? 0.050000 0.000000 0.050000 ( 0.042735) -------------------------------------- total: 1.550000sec user system total real uniq 6.470000 0.430000 6.900000 ( 7.013875) if include? 0.040000 0.000000 0.040000 ( 0.044006) here''s the benchmark code incase I missed anything: --- begin uniq_vs_include.rb require ''benchmark'' n = 5000 Benchmark.bmbm do |b| array = [] 50.times do array << rand(50) end array.uniq items= [] n.times do items << rand(50) end b.report("uniq") do n.times do |i| item = items[i] array << item array.uniq end end b.report("if include?") do n.times do |i| item = items[i] if array.include? item array << item end end end end ---- end uniq_vs_include.rb
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 This seems like the reasonable and expected behavior. Many of my filters are reused as part of higher level DSL methods and my app would become unusable if Rails decided to begin ignoring my explicit instructions and refusing to run filters as I had asked for. It is the programmer''s responsibility to understand the inheritance and behavior of filters within their own app. Rails can''t prevent you from tanking your own performance by asking for expensive filters more than once. I don''t see the need for a :uniq option, when you can use :only and :except on the before_filter and skip_before_filter flavors... Maybe you are seeing deficiencies in your controller designs as flaws in the filter setup? Perhaps you just need to refactor your controllers and use skip and :only/:except more liberally? On Jul 31, 2006, at 11:17 AM, Caio Chassot wrote:> Right now, filters run for as many times as you call, eg., > before_filter. > > If you do something like: > > 5.times { before_filter :foo } > > it might be expect, but I think the more common case of: > > @ ApplicationController > before_filter :foo > > @ SubController > before_filter :foo, :except => :bar > > yields the unexpected result where :foo will run twice. (Luckly, > for sub/bar it doesn''t run at all, which is what I would expect) > > I understand that in this particular case using skip_before_filter > would fix it, but... > > Do you really think it''s a good idea to run filters more than once? > > How about maybe adding a uniq option to pre/append_filter_to_chain, > that would ensure a filter is never added twice (but related > conditions may be changed)? > > It would seem to me most filters that end up running twice or > thrice do so harmlessly, but needlessly. At most, hindering > performance. > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.3 (Darwin) iD8DBQFEziVgqvuZB2zXNU0RAkL/AJ9QfGDqcB3YTOYd0rVecPL0RDB4vACeIrWX StIPZ1Shs8MekLhlugTZGjA=HHW8 -----END PGP SIGNATURE-----
> I don''t see the need for a :uniq option, when you can use :only > and :except on the before_filter and skip_before_filter flavors...I think of it as way to remove the burden from the programmer to think about whether or not the filter was already present in the inheritance chain or not. Specially when you have multiple plugins interacting and setting filters, it can get messy. Also, we can make a parallel to alias_method_chain, think of a filter as a simpler form of: alias_method_chain :perform_action, :filter_name In that case, the chain would never be aliased twice.> Maybe you are seeing deficiencies in your controller designs as > flaws in the filter setup? Perhaps you just need to refactor your > controllers and use skip and :only/:except more liberally?I don''t know. I''m just thinking of the common use case, and looking back I see many times where I had filters that needed to run only once, but could run twice, and no filter at all that I needed to rerun. Do you have any cases where you needed a filter to run more than once? I''m not saying it''s completely useless, just that it''s improbable and that most developers don''t consider this when setting up filters. Also, I think expecitly specifying that you want a filter to rerun is very little work, and may even aid readability. Least surprise and all that.
> > I don''t see the need for a :uniq option, when you can use :only > > and :except on the before_filter and skip_before_filter flavors... > > I think of it as way to remove the burden from the programmer to > think about whether or not the filter was already present in the > inheritance chain or not.Please do come up with some real life examples that skip_* can''t deal with. Unless there is a very compelling set of those, I don''t think its worth it. -- David Heinemeier Hansson http://www.loudthinking.com -- Broadcasting Brain http://www.basecamphq.com -- Online project management http://www.backpackit.com -- Personal information manager http://www.rubyonrails.com -- Web-application framework
On 2006-07-31, at 13:48 , David Heinemeier Hansson wrote:> Please do come up with some real life examples that skip_* can''t deal > with. Unless there is a very compelling set of those, I don''t think > its worth it.I think skip_* can handle it all, I''m just wondering if there are filters running needlessly twice out there. So I coded a quick plugin that''ll help is figure that out, by logging when a filter runs repeatedly. I''d like to ask everyone here with a bit of free time to add this plugin to some of your apps (just unpack to vendor/plugins), browse around a little, or run functional/integration tests, and then grep your RAILS_ENV.log for "called repeatedly" Post here your grep results, even if you find nothing. (So we''ll know it''s not really a problem) _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
> and then grep your RAILS_ENV.log for "called repeatedly"So I tested this on an old app of mine and got this: Filter :check_login called repeatedly ... only this one, a on a few certain pages I dug in and it was due to code similar to example I first showed, using: before_filter :foo, :except => :bar instead of skip_before_filter :foo, :only => :bar In this particular case this is concealed by a DSL-ish class method (do_foo) that wraps the before_filter call. I don''t really think this issue will show up a lot, but I wonder whether it''s common that people end up using *_filter(:except) in place of skip_*(:only)
On 7/31/06, Caio Chassot <lists@v2studio.com> wrote:> > and then grep your RAILS_ENV.log for "called repeatedly" > > So I tested this on an old app of mine and got this: > > Filter :check_login called repeatedly > ... only this one, a on a few certain pages > > I dug in and it was due to code similar to example I first showed, > using: > before_filter :foo, :except => :bar > instead of > skip_before_filter :foo, :only => :bar > > In this particular case this is concealed by a DSL-ish class method > (do_foo) that wraps the before_filter call. > > I don''t really think this issue will show up a lot, but I wonder > whether it''s common that people end up using *_filter(:except) in > place of skip_*(:only)I rarely put before filters in my application controller, and do very little controller inheritance. I don''t see this as an issue at all. -- Rick Olson http://techno-weenie.net
I have the opposite. I have authentication levels monitored by before filters from the application level down and refactored them up from gobs of REST controllers. Rick Olson wrote:> On 7/31/06, Caio Chassot <lists@v2studio.com> wrote: >> > and then grep your RAILS_ENV.log for "called repeatedly" >> >> So I tested this on an old app of mine and got this: >> >> Filter :check_login called repeatedly >> ... only this one, a on a few certain pages >> >> I dug in and it was due to code similar to example I first showed, >> using: >> before_filter :foo, :except => :bar >> instead of >> skip_before_filter :foo, :only => :bar >> >> In this particular case this is concealed by a DSL-ish class method >> (do_foo) that wraps the before_filter call. >> >> I don''t really think this issue will show up a lot, but I wonder >> whether it''s common that people end up using *_filter(:except) in >> place of skip_*(:only) > > I rarely put before filters in my application controller, and do very > little controller inheritance. I don''t see this as an issue at all. >-- Michael Genereux SimianCodex - Web Application Development 949-874-6268
On 2006-07-31, at 15:14 , Rick Olson wrote:> I rarely put before filters in my application controller, and do very > little controller inheritance. I don''t see this as an issue at all.I can''t say I do either all the time, but I do it enough that I noticed this behavior. But I''m not bringing this here as a bug report, instead, as a design question. I''m not concerned, yet, wether this is faulty behavior or whether there''s enough usage/breakage to warranty/not a change. I point to the fact that I ran into this noticing filters running twice, but I never had the need for a filter to run more than once. Was this considered when implementing filters? I suspect this is enough of an edge case that it didn''t matter and never got any attention. So what I''m looking for is either a "indeed, we never thought about it, let''s change/keep it" or "we thought about it and we think running filters multiple times is a great feature"