Rails Ruby on Rails → 2.3.6

Comments and changes to this ticket

• You flagged this item as spam.
  

  dreamcat4 (at gmail) June 7th, 2009 @ 08:28 PM

  • Tag changed from “ruby 1.8.7, activesupport, patch, ruby1.9, tested” to “ruby 1.8.7, activesupport, patch, ruby1.9, tested”

  Error log

  • potionstore-r19-sum-enumerable-error.log 6.3 KB
• 

  Michael Koziarski June 10th, 2009 @ 04:21 AM

  • Assigned user set to “Jeremy Kemper”

  I don't quite follow the test you're adding here.

  Additionally we usually try to avoid doing RUBY_VERSION checks at runtime. Perhaps you could wrap the method definition with that same test?

  Assigning to jeremy as he does the 1.9 stuff

• 

  Jeremy Kemper June 10th, 2009 @ 04:28 AM

  • State changed from “new” to “open”
  • Milestone changed from “2.x” to “2.3.4”

  Test passes for me without the code change on 1.9 svn.

  Could you explain the issue? The tests are confusing.

• You flagged this item as spam.
  

  dreamcat4 (at gmail) June 10th, 2009 @ 09:56 AM

  @Michael Koziarsk (r.e. wrap method in ruby version) - Sure thing, will re-submit a new patch soon. No problem.

  @Jeremy Kemper (r.e. confused because tests don't fail) - Yes thats correct. I saw it only during a rails application startup. Stack trace attached (in the log file).

  The tests given in the patch are simply my best-effort to reproduce the data state at the time of the blow-up. I'd love to be able to write the failing test for this. But coming from a C language background i really don't understand the block handling issues well enough. It all seems a little backwards to me and can't get my head around it.

  But here a very detailed breakdown of what i think is happening:

  1) Rails is Loading up view paths, and scanning directories for files with views/
  layout.rb:176:
  Array(view_paths).sum([]) { |path| Dir["#{path.to_str}/layouts//*"] }
  2) Sum invokes the block each time, which will yield an array of files (in |path|) to add together.
  3) In ruby 1.8, sum will accumulate the result of the block, with inject { |a,b| a + b }
  4) But theres some significant changes to block handling in ruby 1.9. Variable scoping / different behaviour means something is going wrong when the block is returned back in to the sum method. (The sum method is calling itself recursively).
  Now please bear with me here...
  5) Sum method is recursing onto itself, then doing and self.inject { |sum, element| sum + element }. Where the accumulating array object is implied as (self), returned by the previous invokation of sum yielded from the result of the outside block { Dir[] }.
  6) Execution halts during the line "inject { |sum, element| sum + element }".
  7) The data state just before the blow-up was [["file1","file2","file3"],[]]. So i put that in the tests as best representation of the data condition.

  Solution:
  Really the problem is because there are significant changes to block handling in ruby 1.9. Passing variables into blocks become locally scoped, and so on. I don't know exactly whats happening during the recursion. Using more than one blocks recursively just seemed unnecessary here so i chose to do away with the inner block to make simpler for 1.9. Switching to reduce(:+) means we can eliminate that problematic inner block { |sum, element| sum + element } where the blowup occurs. Perhaps it may be because 'sum' declared inside the block is interpreted as a declaration for a new locally scoped variable, instead of a call to self.Enumerable::sum, i don't know. However I feel confident that the patch fixes such error because:

  1) Could only reproduce the error condition by executing my rails app. But when I applied my patch to a clean rails2.3.2 branch then it solved the error outright and with no other code changes.
  2) All the unit tests pass with and without the patch. The purpose of sum function is for adding together or accumulating a list of numbers, strings, or arrays. The passing tests show that all the Numbers, strings and array data types are still working with sum on 1.9 using the new reduce(:+) method instead.
  3) I also tested on 1.8.7, to make sure that it was backwards compatible.

  Please let me know if you need anything else to help get this patch in.

• You flagged this item as spam.
  

  dreamcat4 (at gmail) June 10th, 2009 @ 10:16 AM

  @Michael Koziarsk (r.e. wrap method in ruby version) - Sure thing, will re-submit a new patch soon. No problem.

  @Jeremy Kemper (r.e. confused because tests don't fail) - Yes thats correct. I saw it only during a rails application startup. Stack trace attached (in the log file).

  The tests given in the patch are simply my best-effort to reproduce the data state at the time of the blow-up. I'd love to be able to write the failing test for this. But coming from a C language background i really don't understand the block handling issues well enough. It all seems a little backwards to me and can't get my head around it.

  But here a very detailed breakdown of what i think is happening:

  1) Rails is Loading up view paths, and scanning directories for files with views/**
layout.rb:176: Array(view_paths).sum([]) { |path| Dir["#{path.to_str}/layouts/**/*"] } Sum invokes the block each time, which will yield an array of files (in |path|) to add together.

  2) Sum will try to accumulate the result of the block, with inject { |sum, element| sum + element }
  The sum method is calling itself recursively.

  3) But theres some significant changes to block handling in ruby 1.9. Variable scoping / different behaviour means something is going wrong when the block is being passed back into the sum method.
  Now please bear with me here...

  4) Sum method is recursing onto itself, then doing and inject { |sum, element| sum + element }. The inject function is invoked on the accumulating result object implied as self.inject. This object was returned by the previous invokation of sum and yielded from the result of the outside block { |path| Dir["#{path.to_str}/layouts/**/*"] }.

  5) Execution halts during the line inject { |sum, element| sum + element }.

  6) The data state just before the blow-up was [["string1","string2","string3"],[]]. This was added to the tests because its a best guess representation of the error condition.

  Solution

  This problem showing up because of changes to block handling in ruby 1.9. Passing variables into blocks become locally scoped, and so on. And holding multiple blocks during recursion just seem unnecessary here so my patch will to make simpler for 1.9. Switching to reduce(:+) function means we can eliminate that problematic inner block { |sum, element| sum + element } where the blowup occurs. However I feel confident that the patch fixes such error because:

  1) Could only reproduce the error condition by executing my rails app. But when I applied my patch to a clean rails2.3.2 branch then it solved the error outright and with no other code changes.

  2) All the unit tests pass with and without the patch. The purpose of sum function is for adding together or accumulating a list of numbers, strings, or arrays. The passing tests show that all the Numbers, strings and array data types are still working with sum on 1.9 using the new reduce(:+) method instead.

  3) I also tested on 1.8.7, to make sure that it was backwards compatible.

  Please let me know if you need anything else to help get this patch in.

• 

  dreamcat4 (at gmail) June 10th, 2009 @ 10:44 AM

  • no changes were found...

  • enumerable_sum_for_ruby_19_v2.diff 3.3 KB

• You flagged this item as spam.
  

  dreamcat4 (at gmail) June 10th, 2009 @ 10:44 AM

  Uploaded new patch for ruby 1.9 version check outside of the method body. File is enumerable_sum_for_ruby_19_v2.diff.

• You flagged this item as spam.
  

  Xavier Shay July 31st, 2009 @ 07:01 AM

  I was seeing the same error loading our rails app
  This patch applied cleanly to 2-3-stable @ 17f336e2f00f419a41eb7effb817bd7ad3e84f0d and fixed the error

• 

  Jeremy Kemper September 11th, 2009 @ 11:04 PM

  • Milestone changed from “2.3.4” to “2.3.6”

  [milestone:id#50064 bulk edit command]

• You flagged this item as spam.
  

  Andrew Grim December 15th, 2009 @ 02:55 AM

  I just ran into this error today while looking into 1.9 compat for my app and I think it may be the symptom of a larger issue. When I use Enumerable#sum in my rails app I get the error, but in a default install rails app I don't.

  Digging in some more I noticed that the error was just coming from calling inject with no arguments. Turns out a number of gems/plugins I use define Enumerable#inject when it isn't found (I think it was added in ruby 1.6), but they are checking for it in a manner like:

  unless Enumerable.instance_methods.include?('inject')
  # def include(required_arg) end

  But instance_methods no longer returns strings, it's now an array of symbols. A better method is "Enumerable.method_defined?(:inject)", which works in 1.8 and 1.9 (didn't check anything older).

  The libraries I had trouble with are guid and soap4r, but I'm sure that code has been cargo culted more places than that. While the patch doesn't break anything it's probably not necessary either.

• You flagged this item as spam.
  

  dreamcat4 (at gmail) December 15th, 2009 @ 01:31 PM

  Yeah Andrew is spot on here. I've now found Enumerable::inject overloaded somewhere within my rails project. So thats the reason. Well done! So as Andrew suggests its equally plausible to put a ruby version check around that instead (the overloaded local method). I should get around to trying that sometime.

• 

  Jeremy Kemper April 24th, 2010 @ 08:21 PM

  • State changed from “open” to “invalid”