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

Clean up error handling in the C extension. #2096

Draft
wants to merge 5 commits into
base: main
Choose a base branch
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

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

this will be useful to do some cleanup when we eventually decide to
bump our libxml2 dependency.
and
- remove platform-specific "-D" flags
- remove `vasprintf_free`
- change calls to `vasprintf_free` to call `free`
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

@flavorjones flavorjones mentioned this pull request Oct 14, 2020
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?

@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
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Oct 22, 2021
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 3, 2021
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 3, 2021
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 4, 2021
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Nov 28, 2021
related to exception handling in libxml2 callbacks,
see #2096 for related work
flavorjones added a commit that referenced this pull request Dec 8, 2021
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
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
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 modified the milestones: v1.14.0, v1.15.0 Aug 28, 2022
@flavorjones flavorjones modified the milestones: v1.15.0, v1.16.0 Apr 28, 2023
flavorjones added a commit that referenced this pull request Jul 15, 2024
**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
Labels
topic/memory Segfaults, memory leaks, valgrind testing, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants