-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix limit memory usage of XML::Error.errors
#14966
base: master
Are you sure you want to change the base?
Fix limit memory usage of XML::Error.errors
#14966
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this time the spec is right 😅
|
||
@[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
This patch limits the size of
XML::Error.errors
to contain only the last 10 error messages.A new class property
XML::Error.max_error_capacity
allows configuration of the error capacity. This property is already marked as deprecated because it'll vanish together withXML::Error.errors
.Resolves #14934
Closes #14936