Skip to content

Commit 9696bf0

Browse files
committed
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.
1 parent b5c83a4 commit 9696bf0

File tree

3 files changed

+33
-10
lines changed

3 files changed

+33
-10
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ Next Release
77
* [#1039](https://github.com/intridea/grape/pull/1039): Added support for custom parameter types - [@rnubel](https://github.com/rnubel).
88
* [#1047](https://github.com/intridea/grape/pull/1047): Adds `given` to DSL::Parameters, allowing for dependent params - [@rnubel](https://github.com/rnubel).
99
* [#1064](https://github.com/intridea/grape/pull/1064): Add public `Grape::Exception::ValidationErrors#full_messages` - [@romanlehnert](https://github.com/romanlehnert).
10+
* [#1109](https://github.com/ruby-grape/grape/pull/1109): Memoize Virtus attribute and fix memory leak - [@marshall-lee](https://github.com/marshall-lee).
1011
* Your contribution here!
1112

1213
#### Fixes

lib/grape/validations/validators/coerce.rb

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ module Validations
77
class CoerceValidator < Base
88
def validate_param!(attr_name, params)
99
fail Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message_key: :coerce unless params.is_a? Hash
10-
new_value = coerce_value(@option, params[attr_name])
10+
new_value = coerce_value(params[attr_name])
1111
if valid_type?(new_value)
1212
params[attr_name] = new_value
1313
else
@@ -50,27 +50,36 @@ def valid_type?(val)
5050
end
5151
end
5252

53-
def coerce_value(type, val)
53+
def coerce_value(val)
5454
# Don't coerce things other than nil to Arrays or Hashes
5555
return val || [] if type == Array
5656
return val || Set.new if type == Set
5757
return val || {} if type == Hash
5858

59-
# To support custom types that Virtus can't easily coerce, pass in an
60-
# explicit coercer. Custom types must implement a `parse` class method.
61-
converter_options = {}
62-
if ParameterTypes.custom_type?(type)
63-
converter_options[:coercer] = type.method(:parse)
64-
end
65-
66-
converter = Virtus::Attribute.build(type, converter_options)
6759
converter.coerce(val)
6860

6961
# not the prettiest but some invalid coercion can currently trigger
7062
# errors in Virtus (see coerce_spec.rb:75)
7163
rescue
7264
InvalidValue.new
7365
end
66+
67+
def type
68+
@option
69+
end
70+
71+
def converter
72+
@converter ||=
73+
begin
74+
# To support custom types that Virtus can't easily coerce, pass in an
75+
# explicit coercer. Custom types must implement a `parse` class method.
76+
converter_options = {}
77+
if ParameterTypes.custom_type?(type)
78+
converter_options[:coercer] = type.method(:parse)
79+
end
80+
Virtus::Attribute.build(type, converter_options)
81+
end
82+
end
7483
end
7584
end
7685
end

spec/grape/validations/validators/coerce_spec.rb

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,5 +252,18 @@ class User
252252
expect(last_response.body).to eq('Fixnum')
253253
end
254254
end
255+
256+
context 'converter' do
257+
it 'does not build Virtus::Attribute multiple times' do
258+
subject.params do
259+
requires :something, type: Array[String]
260+
end
261+
subject.get do
262+
end
263+
264+
expect(Virtus::Attribute).to receive(:build).at_most(2).times.and_call_original
265+
10.times { get '/' }
266+
end
267+
end
255268
end
256269
end

0 commit comments

Comments
 (0)