Skip to content
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: XML::Reader XML::Attr garbage collection #2599

Merged
merged 4 commits into from Jul 22, 2022

Conversation

flavorjones
Copy link
Member

@flavorjones flavorjones commented Jul 21, 2022

What problem is this PR intended to solve?

This is a proposed fix for #2598, see that issue for an extended explanation of the problem.

This PR implements "option 2" from that issue's proposed solutions:

  • introduce a new Reader#attribute_hash that will return a Hash<String ⇒ String> (instead of an Array<XML::Attr>)
  • deprecate Reader#attribute_nodes with a plan to remove it entirely in a future release
  • re-implement Reader#attributes to use #attribute_hash (instead of #attribute_nodes)

After this change, only applications calling Reader#attribute_nodes directly will be running the unsafe code. These users will see a deprecation warning and may use #attribute_hash as a replacement.

I think it's very possible that Reader#attribute_hash won't meet the needs of people who are working with namespaced attributes and are using #attribute_nodes for this purpose. However, I'm intentionally deferring any attempt to solve that DX problem until someone who needs this functionality asks for it.

Have you included adequate test coverage?

I tried and failed to add test coverage to the suite that would reproduce the underlying GC bug.

However, existing test coverage of Reader#attributes is sufficient for now.

Does this change affect the behavior of either the C or the Java implementations?

This PR modifies both the C and Java implementations to behave the same.

Notably, the Java implementation contains a small bugfix which is that Reader#namespaces now returns an empty hash when there are no namespaces (it previously returned nil).

@flavorjones flavorjones force-pushed the flavorjones-fix-reader-node-gc branch 4 times, most recently from 945f617 to 42dd5bb Compare Jul 21, 2022
@flavorjones flavorjones marked this pull request as ready for review Jul 21, 2022
@flavorjones flavorjones requested a review from tenderlove Jul 21, 2022
flavorjones added 4 commits Jul 21, 2022
which is now being used to implement Reader#attributes

fix: Reader#namespaces on JRuby now returns an empty hash when no
attributes are present (previously returned nil)
which will use rb_category_warning if it's available, and rb_warning
if it's not
This method will be removed in a future version
@flavorjones flavorjones force-pushed the flavorjones-fix-reader-node-gc branch from 42dd5bb to 113440c Compare Jul 21, 2022
@flavorjones flavorjones requested a review from stevecheckoway Jul 21, 2022
@flavorjones flavorjones merged commit 685a940 into main Jul 22, 2022
109 checks passed
@flavorjones flavorjones deleted the flavorjones-fix-reader-node-gc branch Jul 22, 2022
@flavorjones flavorjones added the topic/memory Segfaults, memory leaks, valgrind testing, etc. label Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/memory Segfaults, memory leaks, valgrind testing, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant