From 9696bf0659782a765cd92fe5ec881ff31bd0771a Mon Sep 17 00:00:00 2001
From: Vladimir Kochnev <hashtable@yandex.ru>
Date: Tue, 18 Aug 2015 21:07:47 +0300
Subject: [PATCH] Memoize Virtus attribute and fix memory leak.

This one fixes a serious problem we faced in production under a heavy
load. Investigation led us to the fact that it happens only when params
are defined with `type: Array[SomeClass]`. In this case `Virtus`
dynamically create some virtual classes (with `Class.new`) which
inherits from base class with `DescendantsTracker` added which saves
descendant classes into the array. Here we have a leak because garbage collector
doesn't free these classes because an array still holds a reference to
them.

So at least I can say that `Virtus` isn't designed for creating things
*dynamically*. They should be stored somehow statically.
---
 CHANGELOG.md                                  |  1 +
 lib/grape/validations/validators/coerce.rb    | 29 ++++++++++++-------
 .../validations/validators/coerce_spec.rb     | 13 +++++++++
 3 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9c572f690d..9ed4d897e2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,7 @@ Next Release
 * [#1039](https://github.com/intridea/grape/pull/1039): Added support for custom parameter types - [@rnubel](https://github.com/rnubel).
 * [#1047](https://github.com/intridea/grape/pull/1047): Adds `given` to DSL::Parameters, allowing for dependent params - [@rnubel](https://github.com/rnubel).
 * [#1064](https://github.com/intridea/grape/pull/1064): Add public `Grape::Exception::ValidationErrors#full_messages` - [@romanlehnert](https://github.com/romanlehnert).
+* [#1109](https://github.com/ruby-grape/grape/pull/1109): Memoize Virtus attribute and fix memory leak - [@marshall-lee](https://github.com/marshall-lee).
 * Your contribution here!
 
 #### Fixes
diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb
index a004119d77..4e7434d18e 100644
--- a/lib/grape/validations/validators/coerce.rb
+++ b/lib/grape/validations/validators/coerce.rb
@@ -7,7 +7,7 @@ module Validations
     class CoerceValidator < Base
       def validate_param!(attr_name, params)
         fail Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message_key: :coerce unless params.is_a? Hash
-        new_value = coerce_value(@option, params[attr_name])
+        new_value = coerce_value(params[attr_name])
         if valid_type?(new_value)
           params[attr_name] = new_value
         else
@@ -50,20 +50,12 @@ def valid_type?(val)
         end
       end
 
-      def coerce_value(type, val)
+      def coerce_value(val)
         # Don't coerce things other than nil to Arrays or Hashes
         return val || []      if type == Array
         return val || Set.new if type == Set
         return val || {}      if type == Hash
 
-        # To support custom types that Virtus can't easily coerce, pass in an
-        # explicit coercer. Custom types must implement a `parse` class method.
-        converter_options = {}
-        if ParameterTypes.custom_type?(type)
-          converter_options[:coercer] = type.method(:parse)
-        end
-
-        converter = Virtus::Attribute.build(type, converter_options)
         converter.coerce(val)
 
       # not the prettiest but some invalid coercion can currently trigger
@@ -71,6 +63,23 @@ def coerce_value(type, val)
       rescue
         InvalidValue.new
       end
+
+      def type
+        @option
+      end
+
+      def converter
+        @converter ||=
+          begin
+            # To support custom types that Virtus can't easily coerce, pass in an
+            # explicit coercer. Custom types must implement a `parse` class method.
+            converter_options = {}
+            if ParameterTypes.custom_type?(type)
+              converter_options[:coercer] = type.method(:parse)
+            end
+            Virtus::Attribute.build(type, converter_options)
+          end
+      end
     end
   end
 end
diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb
index 391e58f593..d05df842ba 100644
--- a/spec/grape/validations/validators/coerce_spec.rb
+++ b/spec/grape/validations/validators/coerce_spec.rb
@@ -252,5 +252,18 @@ class User
         expect(last_response.body).to eq('Fixnum')
       end
     end
+
+    context 'converter' do
+      it 'does not build Virtus::Attribute multiple times' do
+        subject.params do
+          requires :something, type: Array[String]
+        end
+        subject.get do
+        end
+
+        expect(Virtus::Attribute).to receive(:build).at_most(2).times.and_call_original
+        10.times { get '/' }
+      end
+    end
   end
 end