This project is archived and is in readonly mode.

#5720 ✓wontfix
Ben Hollis

ActiveSupport::Benchmarkable.benchmark double-renders

Reported by Ben Hollis | September 28th, 2010 @ 09:01 AM | in 3.1

This can be seen if you create a view that contains:

<% benchmark("Bug") do %>
  Hello
<% end %>
If you view that, you'll see "Hello" printed twice. I'd expect it to only show up once. It doesn't matter if you do it like this either (with an =):
<%= benchmark("Bug") do %>
  Hello
<% end %>

Comments and changes to this ticket

  • Ben Hollis

    Ben Hollis September 29th, 2010 @ 07:35 AM

    Oops, forgot to mention, this is on 3.0.0.

  • Álvaro Bautista

    Álvaro Bautista October 10th, 2010 @ 07:45 PM

    I run your example and I got the same result.

    In fact, it seems to happen when yield is called within any helper method. For instance, having the following helper:

    def wadus
      yield
    end
    

    and the following view:

    <% wadus do %>
      Hello
    <% end %>
    

    the returned string is:

    Hello Hello
    

    Looking at actionpack/lib/action_view/template/handlers/erb.rb and putting some debugger statements I've seen that the resulting code for the block is

    @output_buffer = ActionView::OutputBuffer.new;@output_buffer.append_if_string=  wadus do \n@output_buffer.safe_concat('  Hello\n'); end ;@output_buffer.to_s
    

    The string is concatenated to the output buffer twice, once through safe_concat and another time through append_if_string.

    I think the class ActionView::Template::Handlers::Erubis just extends Erubis::Eruby to use ActionView::OutputBuffer as the output buffer instead of regular strings (or whatever Erubis uses), so it may be just the way Erubis works.

    I've also run the code on 2.3.5 and the string is concatenated to the output buffer just once, so I guess ERB behaves differently in this case.

    I couldn't try in edge because of an annoying problem with arel dependency version.

  • Álvaro Bautista

    Álvaro Bautista October 10th, 2010 @ 11:27 PM

    I kind of found a workaround, it consists of wrapping the yield call with with_output_buffer:

    def wadus
      with_output_buffer { yield }
    end
    

    That implies that anyone writing helpers should know about this and wrap every yield statement to use an auxiliar buffer. Is this a good way of avoiding the double render in these cases?

    Would it be an acceptable solution to alias or extend the ActiveSupport::Benchmarkable#benchmark method in ActionView::Helpers?

    Doing something like:

    module ActionView::Helpers
      def benchmark(message = "Benchmarking", options = {})
        with_output_buffer { super(message, options) }
      end
    end
    

    I wouldn't feel very comfortable with having to change all my yield statements with something { yield }.

    I'm sure this issue must have already been discussed by core members or active committers, so I guess I'm just wandering.

  • Ryan Bigg

    Ryan Bigg October 19th, 2010 @ 08:23 AM

    • Tag cleared.
    • Importance changed from “” to “Low”

    Automatic cleanup of spam.

  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:48 PM

    • State changed from “new” to “open”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:48 PM

    • State changed from “open” to “stale”
  • Aaron Patterson

    Aaron Patterson February 16th, 2011 @ 05:09 AM

    • State changed from “stale” to “open”
    • Assigned user set to “Aaron Patterson”

    Reopening. This is a real bug that exists in 3-0-stable but not master. O_O

  • Ben Hollis

    Ben Hollis February 16th, 2011 @ 08:43 AM

    I futzed around with git bisect, and it looks like ba52748d05da4f95a8f371d628af97b76644bdd3 was the commit that introduced the fix: https://github.com/rails/rails/commit/ba52748d05da4f95a8f371d628af9...

  • Ben Hollis

    Ben Hollis February 16th, 2011 @ 09:00 AM

    It appears that just applying the changes to actionpack/lib/action_view/template/handlers/erb.rb (deleting ActionView::OutputBuffer#append_if_string= and ActionView::Template::Handlers::Erubis#add_stmt) from that commit is enough to fix the bug. Looks like that'll get rid of the deprecation warning too, though.

  • Aaron Patterson

    Aaron Patterson February 25th, 2011 @ 12:43 AM

    • State changed from “open” to “wontfix”
    • Milestone set to 3.1

    Alright, this bug exists because of backwards compatibility with rails 2.3. The problem is that rails 2.3 would concatenate strings with <% erb tags. If you examine the compiled ERb, you'll see:

    1. The body of the yield appends to the output.
    2. The return value of the append call is the value.
    3. The return value of the call is appended to the output.

    The best thing to do in Rails 3.0 is use capture, like this:

    def helper(&block)
      capture(&block)
    end
    

    Removing the backwards compat fixes this problem, so for now, please just use capture. Sorry! :-(

  • Ben Hollis

    Ben Hollis February 25th, 2011 @ 06:40 PM

    Hey Aaron, thanks for taking another look at this. I agree that new helpers should just use capture, but what about existing helpers (and methods that use yield but aren't view-specific), like https://github.com/rails/rails/blob/master/activesupport/lib/active...? Would the recommended usage for that be:

      <%= benchmark("Bug") do %>
        <% capture do %>
          Hello
        <% end %>
      <% end %>
    

    Would it maybe make sense to change that template helper to automatically add the capture to whenever we're using that backwards-compatibility mode?

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages