Skip to content

Fix limit memory usage of XML::Error.errors #14966

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.

Sign up for GitHub

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

Closed
straight-shoota wants to merge 5 commits into from
Closed

Fix limit memory usage of XML::Error.errors #14966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a25c59
Fix limit memory usage of `XML::Error.errors`
straight-shoota Sep 4, 2024
2de986a
Resolve `class_property` to avoid `Deprecated` annotation on the clas…
straight-shoota Sep 4, 2024
7f54377
Test spec
straight-shoota Sep 4, 2024
b889c0f
Fix test
straight-shoota Sep 4, 2024
2e1e8bd
make format
straight-shoota Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions spec/std/xml/reader_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,11 +581,22 @@ module XML
it "adds errors to `XML::Error.errors` (deprecated)" do
XML::Error.errors # clear class error list

reader = XML::Reader.new(%(<people></foo>))
reader.read
reader.expand?
XML::Reader.new(%(<people></foo>)).tap(&.read).expand?
XML::Error.errors.try(&.map(&.to_s)).should eq [
"Opening and ending tag mismatch: people line 1 and foo",
]

%w[foo bar baz qux quux corge grault].each do |tag|
XML::Reader.new(%(<#{tag}></a>)).tap(&.read).expand?
end

XML::Error.errors.try(&.map(&.to_s)).should eq ["Opening and ending tag mismatch: people line 1 and foo"]
XML::Error.errors.try(&.map(&.to_s)).should eq [
"Opening and ending tag mismatch: baz line 1 and a",
"Opening and ending tag mismatch: qux line 1 and a",
"Opening and ending tag mismatch: quux line 1 and a",
"Opening and ending tag mismatch: corge line 1 and a",
"Opening and ending tag mismatch: grault line 1 and a",
]
end
end
end
23 changes: 20 additions & 3 deletions src/xml/error.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,36 @@ class XML::Error < Exception
super(message)
end

@@errors = [] of self
@@max_error_capacity = 5

@[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
Copy link
Copy Markdown
Collaborator

@ysbaddaden ysbaddaden Sep 5, 2024

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 😕

Copy link
Copy Markdown
Member Author

@straight-shoota straight-shoota Sep 5, 2024

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).


@[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

@@errors = Deque(self).new(max_error_capacity)
Copy link
Copy Markdown
Collaborator

@ysbaddaden ysbaddaden Sep 5, 2024

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?

Copy link
Copy Markdown
Member Author

@straight-shoota straight-shoota Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, yeah it will 🤦


# :nodoc:
protected def self.add_errors(errors)
@@errors.concat(errors)
new_errors_size = errors.size.clamp(..max_error_capacity)
remaining_size = max_error_capacity - new_errors_size
(@@errors.size - remaining_size).times { @@errors.shift }

errors.to_unsafe.to_slice(errors.size)[-new_errors_size, new_errors_size].each do |error|
@@errors.push error
end
end

@[Deprecated("This class accessor is deprecated. XML errors are accessible directly in the respective context via `XML::Reader#errors` and `XML::Node#errors`.")]
def self.errors : Array(XML::Error)?
if @@errors.empty?
nil
else
errors = @@errors.dup
errors = @@errors.to_a
@@errors.clear
errors
end
Expand Down