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.

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

Jump to bottom

Clean up error handling in the C extension. #2096

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Clean up error handling in the C extension. #2096

wants to merge 5 commits into from

Conversation

flavorjones
Copy link
Member

This PR is for a set of accumulated changes in the C extension around libxml2 error handling.

I'd like to, whenever possible, avoid raising an exception from a native C callback. This is primarily to do with preventing memory leaks as detailed in #1610, but also to allow TruffleRuby to unwind the stack properly as detailed in #1882. I'm creating it as a draft so that I can get incremental feedback from CI as I go.

@flavorjones flavorjones added the topic/memory Segfaults, memory leaks, valgrind testing, etc. label Oct 12, 2020
@flavorjones flavorjones added this to the v1.11.0 milestone Oct 12, 2020
@codeclimate
Copy link

codeclimate bot commented Oct 12, 2020 •
edited
Loading

Code Climate has analyzed commit 426fd89 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 94.3% (0.0% change).

View more on Code Climate.

@AppVeyorBot
Copy link

✅ Build nokogiri 1.0.707 completed (commit ccebed86e0 by @flavorjones)

@flavorjones
cleanup: no need to check for HAVE_RUBY_UTIL_H in C extension
6ac0384 
which was introduced in ruby v1_9_0_3
@flavorjones
info: add libxml2 version information to extconf have_func calls
17e0439 
this will be useful to do some cleanup when we eventually decide to
bump our libxml2 dependency.
@flavorjones
cleanup: use have_func to check for vasprintf
b329a7c 
and
- remove platform-specific "-D" flags
- remove `vasprintf_free`
- change calls to `vasprintf_free` to call `free`
@flavorjones
cleanup: fix xml_node.c type cast warning
b76ee14
@flavorjones
fix: address memory leak when xpath evaluation raises an error
426fd89 
also ensure CRuby raises an XML::XPath::SyntaxError on xpath function
errors (like JRuby). previously CRuby raised a RuntimeError.

Related to #1610 and #1882.
@flavorjones flavorjones force-pushed the 1610-flavorjones-nokogiri-error-raise-considered-harmful branch from 3bae5cc to 426fd89 Compare October 13, 2020 12:30
@AppVeyorBot
Copy link

✅ Build nokogiri 1.0.710 completed (commit 7ab0da6e4f by @flavorjones)

@flavorjones flavorjones mentioned this pull request Oct 14, 2020
Closed
2 tasks
@flavorjones
Copy link
Member Author

Braindumping some notes here so I don't lose them:

  • create new file libxml2_error_handling.c

  • move some of the error handling routines there

  • experiment with what a wrapper might look like

  • think about the two types of errors

    • what we really want is a SyntaxError with line/col info, and a GenericError
    • should we rename (or alias) XPath::SyntaxError to GenericError?

  • refactor the header files → put it all into nokogiri.h?

@flavorjones flavorjones mentioned this pull request Jan 6, 2021
Closed
Base automatically changed from master to main January 17, 2021 21:53
@LifeIsStrange
Copy link

Any update? :)

@flavorjones
Copy link
Member Author

Hi, @LifeIsStrange! It's great to know people are interested in seeing this work get done. I'm curious why you're interested? Is there a specific problem you're running into that I can help with in the meantime?

@LifeIsStrange
Copy link

hi @flavorjones I'm just a nerd interested in Truffleruby as a technology, and I heard that this issue is a blocker for Ruby on Rails support for truffleruby.
I don't have specific problems as I currently don't use nokogiri, thanks :)

@flavorjones
Copy link
Member Author

@LifeIsStrange thanks for clarifying. To be clear, Nokogiri almost entirely works fine on TruffleRuby, see our CI pipeline here:

These failures are specific to Sulong's lack of support for some types of C callbacks, and can't be fixed without addressing that support. However, the affected functionality is mostly related to error handling during SAX parsing, and so if that's not critical to you, then everything will probably work fine!

If you're having Nokogiri-specific issues running on Rails+TruffleRuby, though, please do let us know in a new issue!

@flavorjones flavorjones modified the milestones: v1.12.0, v1.13.0 Aug 2, 2021
flavorjones added a commit that referenced this pull request Oct 22, 2021
@flavorjones
ci: add a suppression for a known leak
5553e5c 
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Oct 22, 2021
@flavorjones
ci: add a suppression for a known leak
1137d83 
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 3, 2021
@flavorjones
ci: add a suppression for a known leak
eaf53ae 
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 3, 2021
@flavorjones
ci: add a suppression for a known leak
ae49efb 
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 4, 2021
@flavorjones
ci: add a suppression for a known leak
7446e33 
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 28, 2021
@flavorjones
ci: add a suppression for a known leak
04fa154 
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Dec 8, 2021
@flavorjones
ci: todolist the remaining memcheck leak failures
3f4d535 
The remaining leaks are small. Some are from exception handling
detailed at #1610 and being worked on in #2096.

This allows us to detect and new memory leaks introduced.
flavorjones added a commit that referenced this pull request Dec 8, 2021
@flavorjones
ci: todolist the remaining memcheck leak failures
04a7b57 
The remaining leaks are small. Some are from exception handling
detailed at #1610 and being worked on in #2096.

This allows us to detect and new memory leaks introduced.
flavorjones added a commit that referenced this pull request Dec 10, 2021
@flavorjones
ci: todolist the remaining memcheck leak failures
bc597c2 
The remaining leaks are small. Some are from exception handling
detailed at #1610 and being worked on in #2096.

This allows us to detect and new memory leaks introduced.
@flavorjones flavorjones modified the milestones: v1.13.0, v1.14.0 Jan 6, 2022
@flavorjones flavorjones mentioned this pull request Apr 2, 2022
Closed
@flavorjones flavorjones mentioned this pull request Apr 10, 2022
Open
@flavorjones flavorjones modified the milestones: v1.14.0, v1.15.0 Aug 28, 2022
@flavorjones flavorjones mentioned this pull request Sep 29, 2022
Draft
@flavorjones flavorjones modified the milestones: v1.15.0, v1.16.0 Apr 28, 2023
@flavorjones flavorjones mentioned this pull request Jul 15, 2024
Merged
flavorjones added a commit that referenced this pull request Jul 15, 2024
@flavorjones
ci: update suppression to match new stack trace in Ruby 3.4-dev (#3291)
2d0afb8 
**What problem is this PR intended to solve?**

I think the stack trace changed with ruby/ruby@51bd8165, see for example
https://github.com/sparklemotion/nokogiri/actions/runs/9935752042/job/27442553528

I'm not sure why this is the only leak showing up now, but am deferring
further study until I devote some time to cleaning up known leaks where
we raise exceptions in Ruby callbacks (from libxml2), see #2096 and
#1610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
topic/memory Segfaults, memory leaks, valgrind testing, etc.
Projects
None yet
Milestone
v1.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@flavorjones @AppVeyorBot @LifeIsStrange