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

Register C global variables to Ruby GC to avoid problems with GC.compact #1115

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

casperisfine
Copy link
Contributor

Since Ruby 2.7, C global variables holding references to Ruby objects must be declared to the GC even if they are also held from the Ruby side, otherwise a call to GC.compact might move them to another location.

For instance here's an exception we've seen caused by this:

bundler/gems/mysql2-d3c815c68f22/lib/mysql2/client.rb:131:in `_query': NoMethodError: undefined method `new_with_args' for "2":String (ActiveRecord::StatementInvalid)
    from bundler/gems/mysql2-d3c815c68f22/lib/mysql2/client.rb:131:in `block in query'
    from bundler/gems/mysql2-d3c815c68f22/lib/mysql2/client.rb:130:in `handle_interrupt'
    from bundler/gems/mysql2-d3c815c68f22/lib/mysql2/client.rb:130:in `query'

In this instance Mysql2::Error was moved to another location, and cMysql2Error was now pointing to an instance of "2".

We had a similar issue in Shopify/liquid-c#55, however here I tried to reproduce it in the test suite by repeatedly calling GC.compact, but without any success. Maybe the mysql2 test suite is not producing enough GC objects to demonstrate the bug.

cc @csfrancis @XrXr @rafaelfranca

Since Ruby 2.7, C global variables holding references
to Ruby objects must be declared to the GC even if
they are also held from the Ruby side, otherwise
a call to GC.compact might move them to another location.
@sodabrew
Copy link
Collaborator

sodabrew commented Apr 1, 2020

Thank you!

@casperisfine
Copy link
Contributor Author

Welcome. Let me know if there's anything needed to get this merged.

I tried some more tricks to reproduce the bug (generate a lot of garbage before loding the extension & then compact), but still no luck.

I wish there was a way to force the GC to move a particular object, that would be very useful to test this kind of stuff.

@sodabrew
Copy link
Collaborator

sodabrew commented Apr 3, 2020

Do all of the static variables need to be flagged against movement as well? For example

  intern_usec = rb_intern("usec");

@casperisfine
Copy link
Contributor Author

@sodabrew I'll let @XrXr correct me if I say something wrong.

But I didn't mark these symbols, because AFAIK they aren't true objects. Meaning there's nothing more than the content of the VALUE. There's no associated object slot that can be moved.

Same for Integer and Float etc. But other than that, yes all static VALUE should be marked.

@sodabrew sodabrew merged commit ca08712 into brianmario:master Apr 5, 2020
@sodabrew sodabrew added this to the 0.5.4 milestone Apr 5, 2020
@sodabrew
Copy link
Collaborator

sodabrew commented Apr 5, 2020

I'm not terribly familiar with these changes in Ruby 2.7, so I'll take your testing-word for it. If the other variables should also be marked that's easy enough to add in a subsequent PR!

@casperisfine
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants