Fix retaining setup blocks when remounting APIs

[jylamont]

This is a simple fix for #2071.

Grape::API class methods that are not referenced in the NON_OVERRIDABLE constant are overridden to call add_setup when invoked. Unfortunately there is a memory leak with add_setup where a hash describing the class method call is added to @setup on every call (not just the first). This PR changes the data structure of @setup from an Array to a Set therefore only allowing a single hash per combination of method name and arguments.

This bug is causing pain for us as we call Grape::API.logger in every API request 📈 In our case Grape::API.logger with no arguments is sorted by moving to a Set, but I am not too familiar with the inner workings of Grape so I will ask for some guidance of whether this doesn't meet a particular edge case or breaks some other functionality. Also the spec I have added was for our specific issue, so I ask for guidance if you guys would prefer a more generic spec somewhere else.

[dblock]

Let's convince ourselves that it's not going to cause side effects during remounting, cc: @myxoh?

Take the example from #1803. Mounting twice causes a single entry in Set, right? What could go wrong?

[dblock]

I'm having hard time demonstrating the leak myself, with or without remounting. Help me modify /~https://github.com/dblock/grape/blob/leak-2102/benchmark/remounting.rb please?

[jylamont]

Working on it! We definitely saw memory usage normalize when I deployed this patch to production but it will be good to demonstrate in a test like what you are doing.

[jylamont]

I changed the script to this:

$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib'))
require 'grape'
require 'benchmark/memory'

class VotingApi < Grape::API
  logger Logger.new(STDOUT)

  helpers do
    def logger
      VotingApi.logger
    end
  end

  namespace 'votes' do
    get do
      logger
    end
  end
end

class PostApi < Grape::API
  mount VotingApi
end

class CommentAPI < Grape::API
  mount VotingApi
end

env = Rack::MockRequest.env_for('/votes', method: 'GET')

PostApi.call(env)
CommentAPI.call(env)

setup_ar = VotingApi.instance_variable_get(:@setup).to_a
setup_set = VotingApi.instance_variable_get(:@setup).to_set

Benchmark.memory do |api|
  calls = 1000

  api.report('using Array') do
    VotingApi.instance_variable_set(:@setup, setup_ar)
    calls.times { PostApi.call(env) }
  end

  api.report('using Set') do
    VotingApi.instance_variable_set(:@setup, setup_set)
    calls.times { PostApi.call(env) }
  end

  api.compare!
end

And get the following output:

Calculating -------------------------------------
         using Array    21.261M memsize (   274.472k retained)
                       178.001k objects (     2.012k retained)
                        18.000  strings (     0.000  retained)
           using Set    21.261M memsize (     2.472k retained)
                       178.001k objects (    12.000  retained)
                        18.000  strings (     0.000  retained)

Comparison:
         using Array:   21261040 allocated
           using Set:   21261040 allocated - same

I think the key thing to pay attention to is the retained objects for each run. We are still creating a hash on every call to add_setup so it makes sense that the same number of objects would be created, but when using a Set these objects are discarded immediately. I guess Comparison will only show something useful if we do not create the hashes at all.

[dblock]

I'm good with merging this after:

1. Squash your changes, see my nitpicks below.
2. Merge /~https://github.com/dblock/grape/tree/leak-2102 on top into this PR, let's keep the benchmark.

[jylamont]

@dblock Thanks for those suggestions. I've made them and squashed the commits. Let me know if you need anything else 👍

[eflukx]

We've had some breakage due to this change (i.e. everything works as expected in d40d697)

We use a simple API builder component to setup our mundane CRUD end-points. Code example that breaks after this merge:

module PortalMounts

  class MyAPIBuilder < Grape::API

    def self.build_api!

      api_self = self

      get 'works' do
        'this also works!'
      end

      resource "things" do
        get 'also_works' do
          'this also works!'
        end

        api_self.add_endpoint

      end
    end

    private

    def self.add_endpoint
      get 'not_work' do
        'broken endpoint!'
      end
    end

  end

  class MyAPI < MyAPIBuilder
    build_api!
  end

end

MyAPI is the class mounted, end points are added through MyAPIBuilder. The call to .add_endpoint fails with RuntimeError: can't add a new key into hash during iteration.

Workaround is found by passing the 'resource self' explicitly, like this:

class MyAPIBuilder < Grape::API

  def self.build_api!

    api_self = self

    get 'works' do
      'this also works!'
    end

    resource "things" do
      get 'also_works' do
        'this also works!'
      end

      api_self.add_endpoint self

    end
  end

  private

  def self.add_endpoint resource_self
    resource_self.get 'not_work' do
      'broken endpoint!'
    end
  end

end

and calling grape methods on resource_self explicitly.

I can't say if this is behavior is desired or a regression?

[dblock]

I can't say if this is behavior is desired or a regression?

i don't think anything that used to work should be desired ;) so let's call it a regression

PR a spec and let's see how bad the fix could be?