Memory Leak since 1.2.0

[jsteinberg]

There is a memory leak related to remounting apis. It was originally addressed here.

I believe this is an incomplete solution. The root of the issue is calling the add_setup method every time a class method is called on a Grape::API subclass instead of on a Grape::API::Instance subclass.

The way this is coded up, we are going have have to keep dealing with this issue one method at a time.

It also can happen with methods created outside of the grape gem if you follow the instructions in the upgrading guide, and unless you do some really hack ruby stuff to unfreeze the collection and add your methods, there is no way to get them included in the blacklist.

Here is a quick example of something that causes the leak.

App::API.instance_variable_get(:@setup).length
=> 27

1000.times do 
  App::API.versions
end;

 App::API.instance_variable_get(:@setup).length
=> 1027

To inspect a full list of affected methods, just do this in your closest grape based app.

(App::API.base_instance.methods - Grape::API::NON_OVERRIDABLE).sort

A quick list of notable methods that stick out to me from that list are:

base, endpoints, logger, instance, versions...

Basically, any method that is not part of the DSL for building/describing a Grape::API::Instance.

Because of this issue, his line from the upgrading guide is not factual.

This changes were done in such a way that no code-changes should be required.

I believe this needs to be address properly or that line needs to change to warn users about invoking methods on a Grape::API class.

I'm not familiar enough with the code to make a real suggestion for fixing it, but I dont think this is a very sane way to "copy" an object. Also, in the switch from being class based to "class instance"(subclasses) based, I think someone forgot about actual instances.

The DSL's build up state that should be clonable when creating the "class instances". Alternatively, instead of a blacklist of methods, potentially there could be a whitelist of methods that are part of the DSL for describing a Grape::API::Instance.

[dblock]

Thanks for the analysis @jsteinberg.

[dblock]

cc: @myxoh

[dnesteryuk]

What if we fix it this way? Yeah, API.compile! will be mandatory in this case, however, we might clean up some things, for example, @setup variable 🤔

[dblock]

What if we fix it this way? Yeah, API.compile! will be mandatory in this case, however, we might clean up some things, for example, @setup variable 🤔

You mean users will all have to call API.compile!? I don't think we can get away with that.

[myxoh]

Hi @jsteinberg - apologize I'm so late to the party as this is definitely my mess.

I'm not familiar enough with the code to make a real suggestion for fixing it, but I dont think this is a very sane way to "copy" an object. Also, in the switch from being class based to "class instance"(subclasses) based, I think someone forgot about actual instances.

I do agree the current approach is messy, I couldn't think of any other way of creating the "remount" logic without breaking the pre-exisitng DSL on update.

I can think of two (three) approaches to get us out of this mess - both of them involve some form of breaking change:

Approach number 1
We fix this specific issue.

We could easily do something like [We would need to define the name of the method]

      def perform_a_one_off_operation
        yield(self)
      end

on the grape/api.rb class

(Where then the code snippet that caused the memory leak should be updated to):

App::API.instance_variable_get(:@setup).length
=> 27

1000.times do 
  App::API.perform_a_one_off_operation(&:versions)
end;

 App::API.instance_variable_get(:@setup).length
=> 27

Approach number 2

We untangle the mess for remounting and contain all of the "setup" logic under an explicit set up
(Benefit - it probably would result in more clean code internal;
Con - it probably would impact a lot more people)

We change the DSL so you have to do something like

class MyAPI << Grape::API
  define_on_mount do
    get '....'
  end
end

Then it's a lot easier to basically remember the routes that were defined by the API (we only evaluate that block on mount, so we can get rid of all the "setup" code I've introduced and end up with a cleanr approach)

I'm sure there are plenty of alternatives, these 2 are off the top of my head

NOTE

I am not super sure what is the real world scenario in which versions gets called outside the "setup" phase on the base class.
There might be a more optimal solution than 1 and 2 to address the specific needs here.

---

BTW I readily admit the internals are messy, and I would love any suggestions that would remove the current approach and introduce something saner

[dblock]

Some ideas I haven't tried:

• flip NON_OVERRIDABLE to OVERRIDABLE
• include a spec that keeps ^ in sync

[myxoh]

Moving to "OVERRIDABLE" would work I pressume as well, but I'd be a pain to keep it in sync with the DSL (and also, if someone added extensions to the DSL, they would have to add to that)
Not against that option TBH

[jylamont]

Are there any downsides to changing @setup = [] to @setup = Set.new? We are going to monkey patch this to fix the leak, but I am unfamiliar with the inner workings of Grape and not sure if this will break something else.

Also want to note that if you follow the recommendation in the README for setting up logging, you will run into this leak.

class API < Grape::API
  helpers do
    def logger
      API.logger
    end
  end

  post '/statuses' do
    logger.info "#{current_user} has statused"
  end
end

[dblock]

Are there any downsides to changing @setup = [] to @setup = Set.new? We are going to monkey patch this to fix the leak, but I am unfamiliar with the inner workings of Grape and not sure if this will break something else.

This seems too simple :) Does this actually work for remounting? I don't see a downside if all specs pass and it fixes the problem, but I feel like I'm missing something - @myxoh?

[jylamont]

I haven't gotten into re-mounting yet, but I can confirm all specs pass. Hopefully the fix is that simple! I can make a PR next week if that is helpful.

[dblock]

Yes, please @jylamont.

[masukomi]

Does #2102 fix this completely or just partially? I ask because this is still open and the fix wasn't mentioned in the release notes for 1.5.0

[jsteinberg]

From a quick glance it looks like it 'fixes' the memory leak in my simple example.

I still believe this is an incorrect and error prone approach for copying state. I think adding versions(or any more realistic method like instance | logger) to the set on every call is still not great. But the 'leak' appears fixed.

If we look at my actual example that lead me to this issue, you can see that it still is not really fixed.

# leak
route = grape_app.match? env

# no leak
route = grape_app.base_instance.match? env

or something using the logger:

# leak
grape_app.logger.info(Time.now)

So, any non setup method that has variable arguments is going to still display this behavior.

setup_step = { method: method, args: args, block: block }
@setup << setup_step

[dblock]

@jsteinberg what do you propose?

[jsteinberg]

The grape DSL builds up state. Instead of attempting to replay those DSL calls to rebuild state, I think it makes more sense to copy(deep copy) the built state. I understand this is a significant re-write which is why I haven't attempted to tackle this myself. Unfortunately I am too busy currently to do this(maybe I can find some time over the holidays).

A whitelist of methods that are actual DSL methods to me makes more sense than currently implemented blacklist. I have not dug into the internals of grape enough to feel confident in creating this list.

I suppose an alternate quicker approach would instead allow developers to opt in to this remounting behavior instead of having it be the default. Then anyone looking to do this could be better made aware of the side effects via documentation.

So, a DSL method called something like remountable! that tracks the methods called, and the default behavior is they are not tracked.

Downside to this approach is it would be a breaking change. But I would be willing to bet the number of people re-mounting is significantly lower than the number not(especially for legacy code bases).

Maybe if there was a global setting that controlled default behavior, we could get away with a non breaking change. Global setting would allow a developer to set all APIs by default to not remountable, and then they can individually re-enable remountabilty. But honestly I feel like the default should be opt-out, so maybe breaking change is the way to go.

[dblock]

Thanks @jsteinberg. I dislike the mental overhead for developers. Maybe we can just better document side effects of remounting in README?

[jsteinberg]

The side effects(leak) are present regardless on wether you are remounting or not.

[dblock]

The side effects(leak) are present regardless on wether you are remounting or not.

Right. Maybe we can make more methods private? I'm just spitballing, I don't have a good solution.

[jsteinberg]

Sorry, I just wanted to make sure as your comment about documentation read differently to me.

Maybe just leave this issue open for now to keep it visible and I'll try to get around to coding up a solution when I have some more time.