100% CPU when comparing ActiveController::TestResponse with a primitive

[timothyandrew]

We ran into a bug today which pushed our CPU up to 100% and forced us to restart our Mac.

Our test case was something like:

it "returns all the info for a User in JSON excluding password" do
  user = FactoryGirl.create(:user)
  get :show, :id => user.id
  response.should == user.as_json(except: :password_digest)
end

This is a mistake…we intended to test response.body, not response. However running this code made the machine completely unresponsive, with the CPU up at 100%.

When we investigated this further (on multiple macs), it seems like comparing a response with any Ruby primitive seems to freeze the test and push the CPU up to 100%.

response.should == {} # 100% CPU
response.should == []  # 100% CPU
ActiveController::TestResponse.new.should == {}  # 100% CPU
ActiveController::TestResponse.new.should == ".."  # 100% CPU
ActionDispatch::TestResponse.new.should == {}  # 100% CPU
request.should == {} # Test runs fine
response.body.should == {} # Test runs fine

Is this an RSpec bug?

Best,
@timothyandrew & @Who828

[dchelimsky]

The bug is in rack. See rspec/rspec-expectations#166 and rack/rack#419.

[zmoazeni]

I don't know if it makes sense to comment on this issue or file a new one. Let me know what you folks prefer

In Rails 4.0, the source of this bug is no longer in Rack. It's in Rails https://github.com/rails/rails/blob/4ac114471cc14de44b77ced3dd4a0676c13b818b/actionpack/lib/action_dispatch/http/response.rb#L251-L253 depends on https://github.com/rails/rails/blob/4ac114471cc14de44b77ced3dd4a0676c13b818b/actionpack/lib/action_dispatch/http/response.rb#L316 which causes the infinite recursion. Let me know if you'd like a sample codebase.

A backtrace points back to https://github.com/rspec/rspec-expectations/blob/9003d2d7ef0c124f06f135a436d84c031947a608/lib/rspec/expectations/fail_with.rb#L42 as the source of the recursion.

/cc @alindeman @myronmarston since you both have responded to previous issues.

[myronmarston]

Arrgggg. IMO, this is a bug in rails and should be reported there and fixed upstream. Objects that cause infinite recursion when flatten is sent to an array containing the object are broken and should be fixed.

[zmoazeni]

Arrgggg. IMO, this is a bug in rails and should be reported there and fixed upstream.

@myronmarston I respectfully disagree for a few reasons.

1. This issue has cropped up from multiple upstreams now (first rack and now rails).
2. While expect(response).to eq(status_code) is incorrect, it's an easy mistake to make. And the fact that the process hangs with no explanation is a big bummer and time sink to the developer who has no feedback that something is wrong.
3. flatten in this context seems to be just used in helper methods to decide how best to print out the error for the user. This is a hackish helper that could help do the same thing:

def recursive_safe_flatten(array, final = [], seen = Set.new)
  array.each do |e|
    case e
    when String, Numeric, Symbol
      # these are "basic" types that we can safely append and move on
      final << e 
    else
      e_id = e.object_id
      if seen.include?(e_id)
        # raised in the event the same instance found in sub-arrays.
        # instead of raising an error, this could just add the second instance
        #
        # this probably needs to be tweaked in the current context where rspec 
        # could call it like recursive_safe_flatten([actual, expected])
        raise ArgumentError.new("recursive reference found: #{e.inspect}")
      else
        # We haven't see this object before, let's add it to our "seen" registry
        seen.add(e_id)

        if e.respond_to?(:flatten)
          # this is a flatten-able object, let's recurse through it appending
          # elements as we go
          reference_flatten(e, final, seen)
        else
          # this is not a flatten-able object, let's append it and move on
          final << e
        end
      end
    end
  end
  final
end

# used like:
recursive_safe_flatten(possibly_recursive_array)

Even if it's not perfect, rspec could fail fast with a better error message to point the dev in the right direction.

I wrote this by looking at the MRI v2.1's Set#flatten and unsuccessfully trying to grok Array#flatten.

[zmoazeni]

I updated the snippet above with comments to make it a little easier to grok the logic.

[myronmarston]

@myronmarston I respectfully disagree for a few reasons.

You disagree that this is a bug in rails? How can an object that causes Array#flatten to recurse infinitely not be considered a bug?

[zmoazeni]

You disagree that this is a bug in rails? How can an object that causes Array#flatten to recurse infinitely not be considered a bug?

I don't think self-referential collections are necessarily an evil thing. But I feel like that's a different debate and I don't have enough experience with Rack to debate whether Rack Responses should not recurse.

My point is that is that rspec is currently depending on Array#flatten in order to give the rspec developer/user better failure messages to help make their lives easier when debugging a test failure. In this case, a never-ending program with no output is on the extreme end of making things harder for the dev/user to figure out what's going on. A small utility method similar to one I suggested above is a decent compromise.

[myronmarston]

I don't think self-referential collections are necessarily an evil thing.

I've never seen a use for arrays that contain themselves, but that actually wouldn't cause the infinite recursion spike:

irb(main):001:0> k = [1]
=> [1]
irb(main):002:0> k << k
=> [1, [...]]
irb(main):003:0> k
=> [1, [...]]
irb(main):004:0> k.flatten
ArgumentError: tried to flatten recursive array
    from (irb):4:in `flatten'
    from (irb):4
    from /Users/myron/.rubies/ruby-1.9.3-p448/bin/irb:12:in `<main>'

[myronmarston]

@zmoazeni -- I've submitted a fix to rails -- see rails/rails#13921.

[myronmarston]

@zmoazeni -- a fix has been merged into rails: rails/rails#13982.

[dchelimsky]

👍 👍 @myronmarston 👍 👍

[zmoazeni]

@myronmarston Thanks for driving it forward