Skip to content

Commit

Permalink
Memoize Virtus attribute and fix memory leak.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
marshall-lee committed Aug 18, 2015
1 parent b5c83a4 commit 9696bf0
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 10 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 19 additions & 10 deletions lib/grape/validations/validators/coerce.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -50,27 +50,36 @@ 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
# errors in Virtus (see coerce_spec.rb:75)
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
13 changes: 13 additions & 0 deletions spec/grape/validations/validators/coerce_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 9696bf0

Please sign in to comment.