Grails

Memory Leak in DefaultUrlMappingsHolder

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Fixed
  • Affects Version/s: 1.3.4
  • Fix Version/s: 1.3.5
  • Component/s: View technologies
  • Labels:
    None
  • Environment:
    Linux, Grails 1.3.4, running with JMeter test
  • Testcase included:
    yes

Description

A memory leak has surfaced while testing a web service that I am writing. I rely heavily on params.id being fused into the url when making requests
e.g. http://localhost:8080/some/action/1234 would yield params.id == 1234

I have found that every single unique request url is being stored in the DefaultUrlMappingsHolder without ever being cleared up. This causes the app to run out of Old Gen space very quickly, and ultimately causes the app to hang completely.

This problem is very easy to replicate. I have attached a small example Grails app, with a JMeter jmx file in the root of the project which will replicate this issue in a short period of time. I've been using jvisualvm to monitor the heap with it's Visual GC perspective.

This is a serious problem for us, as this blocks us from putting this app into production soon.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Burt Beckwith added a comment -

Looks like replacing the two cache maps with https://code.google.com/p/concurrentlinkedhashmap/ with a max size would achieve a good balance between recomputing lookups and memory usage.

Show
Burt Beckwith added a comment - Looks like replacing the two cache maps with https://code.google.com/p/concurrentlinkedhashmap/ with a max size would achieve a good balance between recomputing lookups and memory usage.
Hide
Permalink
Marco Vermeulen added a comment -

Looks like the tarball was corrupt. Reloaded fresh version.

Show
Marco Vermeulen added a comment - Looks like the tarball was corrupt. Reloaded fresh version.
Hide
Permalink
Marco Vermeulen added a comment -

I've rolled a fix for this issue. Tested it locally and all seems to work fine.

I've forked the grails-core repo at:
git@github.com:marcoVermeulen/grails-core.git

I've made a single commit with all my changes:
http://github.com/marcoVermeulen/grails-core/commit/e1fefb483c0d06dfa02e3b97877cc8a1a7e1162f

Would someone perhaps be able to peer review? If you approve, then cherry-pick it across to the main repo?

Show
Marco Vermeulen added a comment - I've rolled a fix for this issue. Tested it locally and all seems to work fine. I've forked the grails-core repo at: git@github.com:marcoVermeulen/grails-core.git I've made a single commit with all my changes: http://github.com/marcoVermeulen/grails-core/commit/e1fefb483c0d06dfa02e3b97877cc8a1a7e1162f Would someone perhaps be able to peer review? If you approve, then cherry-pick it across to the main repo?
Hide
Permalink
Burt Beckwith added a comment -

Better to do a pull request. That way your commit will get pulled into the main repo rather than the simple copy that cherry picking creates.

Show
Burt Beckwith added a comment - Better to do a pull request. That way your commit will get pulled into the main repo rather than the simple copy that cherry picking creates.
Hide
Permalink
Marco Vermeulen added a comment -

I've sent out a pull request. Please let me know if there are any problems with this.

Show
Marco Vermeulen added a comment - I've sent out a pull request. Please let me know if there are any problems with this.
Hide
Permalink
Marco Vermeulen added a comment -

I've been chatting to Ben Manes, the author of the ConcurrentLinkedHashMap regarding my current implementation. He suggested I use a ListWeigher when building the Map<String,List<UrlMappingInfo>> cache instance. The weigher considers not only the size of the map entries, but also of each element inside the List too. Very handy.

The only problem I experienced was that he constrained the WeightValue to always be bigger than 0. Only problem, the ListWeigher uses the size of the List as it's weight. Which means that it will blow up when putting an empty list into the map (which the DefaultUrlMappingsHolder does!).

As a result, I had to write a CustomListWeigher, which guarantees that an empty list equates to 1, with each subsequent element added incrementing the weight.

I've pushed this to my forked repo as before.

Show
Marco Vermeulen added a comment - I've been chatting to Ben Manes, the author of the ConcurrentLinkedHashMap regarding my current implementation. He suggested I use a ListWeigher when building the Map<String,List<UrlMappingInfo>> cache instance. The weigher considers not only the size of the map entries, but also of each element inside the List too. Very handy. The only problem I experienced was that he constrained the WeightValue to always be bigger than 0. Only problem, the ListWeigher uses the size of the List as it's weight. Which means that it will blow up when putting an empty list into the map (which the DefaultUrlMappingsHolder does!). As a result, I had to write a CustomListWeigher, which guarantees that an empty list equates to 1, with each subsequent element added incrementing the weight. I've pushed this to my forked repo as before.
Hide
Permalink
Lari Hotari added a comment -

I've applied the commits to the master branch:
http://github.com/grails/grails-core/commit/55cf993bc0aec4e24fe181fa7d2a34c4fd94eacf
http://github.com/grails/grails-core/commit/9c6dad3b974a26743f58961ce98234ae25799030
http://github.com/grails/grails-core/commit/f9c371e5a684fedab562cd418c956de820b2959c
http://github.com/grails/grails-core/commit/165fec868d0fc93023eba771d5a213cedfd6f361
http://github.com/grails/grails-core/commit/25fa52de74ed667072c4eac98c5ebb124fe85d21
http://github.com/grails/grails-core/commit/fc1091ac58af33123e6440ad0d58b8007e33f038

I made some small changes to the cache size configuration in the last commit.

Show
Lari Hotari added a comment - I've applied the commits to the master branch: http://github.com/grails/grails-core/commit/55cf993bc0aec4e24fe181fa7d2a34c4fd94eacf http://github.com/grails/grails-core/commit/9c6dad3b974a26743f58961ce98234ae25799030 http://github.com/grails/grails-core/commit/f9c371e5a684fedab562cd418c956de820b2959c http://github.com/grails/grails-core/commit/165fec868d0fc93023eba771d5a213cedfd6f361 http://github.com/grails/grails-core/commit/25fa52de74ed667072c4eac98c5ebb124fe85d21 http://github.com/grails/grails-core/commit/fc1091ac58af33123e6440ad0d58b8007e33f038 I made some small changes to the cache size configuration in the last commit.
Hide
Permalink
Marco Vermeulen added a comment -

Changed the ConcurrentLinkedHashmap dependency to the jdk 1.5 compliant version.

Show
Marco Vermeulen added a comment - Changed the ConcurrentLinkedHashmap dependency to the jdk 1.5 compliant version.
Hide
Permalink
Graeme Rocher added a comment -

Applied. Thanks!

Show
Graeme Rocher added a comment - Applied. Thanks!

People

  • Assignee:
    Lari Hotari
    Reporter:
    Marco Vermeulen
Vote (5)
Watch (10)

Dates

  • Created:
    Updated:
    Resolved: