Fix memory leak caused by caching all unique paths seen #2084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generated by 🚫 danger |
dblock
reviewed
Jul 10, 2020
CHANGELOG.md
Outdated
| @@ -6,6 +6,7 @@ | |||
| * [#2060](https://github.com/ruby-grape/grape/pull/2060): Drop support for Ruby 2.4 - [@dblock](https://github.com/dblock). | |||
| * [#2060](https://github.com/ruby-grape/grape/pull/2060): Upgraded Rubocop to 0.84.0 - [@dblock](https://github.com/dblock). | |||
| * [#2077](https://github.com/ruby-grape/grape/pull/2077): Simplify logic for defining declared params - [@dnesteryuk](https://github.com/dnesteryuk). | |||
| * [#2084](https://github.com/ruby-grape/grape/pull/2084): Fix memory leak in path normalization- [@fcheung](https://github.com/fcheung). | |||
Closed
|
Also cc: @ericproulx you'll want to take a look at this. |
This optimisation was introduced in ruby-grape#2002. If an api's paths include an id in the path then this cache can grow in an unbounded manner.
|
Thanks - I have fixed changelog. Running 1.3.3 + this patch on one of our hosts at the moment & memory usage seems stable enough. |
|
I don't mind reverting. I'll try to find a way to keep the most unique api calls instead of just adding all of them. Creating a kind of LRU policy but inside a ruby hash. |
terrchen
pushed a commit
to terrchen/gitlab
that referenced
this pull request
Jul 17, 2020
Grape v1.4.0 fixes a memory leak that was present in v1.3.3 (ruby-grape/grape#2084). Full list of changes: * https://github.com/ruby-grape/grape/blob/master/CHANGELOG.md * https://github.com/ruby-grape/grape/compare/v1.3.3..v1.4.0
Merged
dslh
pushed a commit
to dslh/grape-jr
that referenced
this pull request
Jul 22, 2020
Besides upgraded Grape, it changes the response body for 204 HTTP status, now it is an empty string instead of an empty hash. Rack introduced lazy body initialization (rack/rack@8c62821) which only works with strings. However, when 204 HTTP status is returned, Grape doesn't stringify the response (https://github.com/ruby-grape/grape/blob/master/lib/grape/middleware/formatter.rb#L27-L29), thus, a hash is returned as a response body. `Rack::MockResponse` fails with TypeError (no implicit conversion of Hash into String) It could be a bug in the JsonAPI resources gem, why does it return an empty hash for 204 HTTP status?!. Definetly, it isn't a bug in Grape, nor Rack. The logic for formatting the reponse body in Grape hasn't changed for 5 years. The main motivation for this upgrade is a fix for memory leak ruby-grape/grape#2084
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I recently updated one of our apps from grape 1.2.5 to 1.3.3 and have observed a memory leak. After analysing some heap dumps taken from our app I believe I have tracked this back to part of the changes in #2002
The change to router.rb caches normalized paths here: https://github.com/ruby-grape/grape/blob/master/lib/grape/router.rb#L10
This retains indefinitely every path that flows through grape. We have a number of apis where the path is of the form
foo/bar/id/actionwhere id is the id of the object the api call is acting on, so this grows in a more or less unbounded way.Since this was just a memory optimization, I would suggest just reverting that part of the changes in #2002 which is what this PR does