Fix limit memory usage of XML::Error.errors#14966
Fix limit memory usage of XML::Error.errors#14966straight-shoota wants to merge 5 commits intocrystal-lang:masterfrom
XML::Error.errors#14966Conversation
straight-shoota
added
kind:bug
beta-ziliani
left a comment
beta-ziliani
left a comment
There was a problem hiding this comment.
Assuming this time the spec is right 😅
straight-shoota
added
the
breaking-change
|
|
||
| @[Deprecated("This class property is deprecated. XML errors are accessible directly in the respective context via `XML::Reader#errors` and `XML::Node#errors`.")] | ||
| def self.max_error_capacity=(@@max_error_capacity) | ||
| end |
There was a problem hiding this comment.
Question: if I want to disable the behavior, which is the way forward, I have to set it this value to 0, but then I get a deprecation notice 😕
There was a problem hiding this comment.
Hm, I don't think you should actually need to disable it explicitly.
You should of course be able to do that if you want. And in that case this method should not raise a deprecation message. But we want to get rid of this method as well, so it should still be deprecated.
Maybe we should mark it as Experimental instead? 🤔
Or don't annotate it at all but document it that it should always be guarded by compare_version(Crystal::VERSION, "1.13.2") > 0) && compare_version(Crystal::VERSION, "2.0.0") < 0).
| @@max_error_capacity | ||
| end | ||
|
|
||
| @@errors = Deque(self).new(max_error_capacity) |
There was a problem hiding this comment.
Question: this initializer will run before user code, won't @max_error_capacity always be the default 5, whatever I set?
There was a problem hiding this comment.
Argh, yeah it will 🤦
|
This is limiting the leak (it won't keep growing) but it' still allocating pointless strings that will have to be collected... for how long are we gonna keep this? I'm leaning more to have |
|
Yeah, I'm all in for disabling the feature entirely. This patch is just adding more complexity to something we need to get rid of anyway. |
|
Should this be closed in favor of #14936 ? |
4 participants
This patch limits the size of
XML::Error.errorsto contain only the last 10 error messages.A new class property
XML::Error.max_error_capacityallows configuration of the error capacity. This property is already marked as deprecated because it'll vanish together withXML::Error.errors.Resolves #14934
Closes #14936