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 a GC compaction issue with busy_handler #466

Merged
merged 1 commit into from Jan 9, 2024

Conversation

byroot
Copy link
Contributor

@byroot byroot commented Jan 9, 2024

Previous discussion in #458

Storing VALUE self as context for rb_sqlite3_busy_handler is unsafe because as of Ruby 2.7, the GC compactor may move objects around which can lead to this reference pointing to either another random object or to garbage.

Instead we can store the callback reference inside the malloced struct (sqlite3Ruby) which can't possibly move, and then inside the handler, get the callback reference from that struct.

This however requires to define a mark function for the database object , and while I was at it, I implemented compaction support for it so we don't pin that proc.

cc @flavorjones @tenderlove

NULL,
deallocate,
database_memsize,
.wrap_struct_name = "SQLite3::Backup",
Copy link
Contributor

Choose a reason for hiding this comment

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

A question from ignorance:

Peter's article describes the wrap_struct_name this way:

This is a C string name we give for our TypedData object. This name is used for debugging and statistics purposes. Although there are no requirements for this name (it just has to be a valid C string that’s null-terminated), it is a good idea to give it an informative and unique name to make it easy for yourself and other developers to identify the object.

For my own edification, why do we use the same string for 3 different TypedData objects:

https://github.com/search?q=repo%3Asparklemotion%2Fsqlite3-ruby%20rb_data_type_t&type=code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea. I personally tend to set it as the name of the class it backs, but as Peter says, it can be anything.

Ultimately this doesn't have a big impact, the only way to see it is via ObjectSpace.dump and ObjectSpace.dump_all so is only useful when debugging memory related things.

Would probably be worth reworking them all, but I don't own that gem, so not my call.

Previous discussion in sparklemotion#458

Storing `VALUE self` as context for `rb_sqlite3_busy_handler` is unsafe
because as of Ruby 2.7, the GC compactor may move objects around
which can lead to this reference pointing to either another random
object or to garbage.

Instead we can store the callback reference inside the malloced
struct (`sqlite3Ruby`) which can't possibly move, and then inside
the handler, get the callback reference from that struct.

This however requires to define a mark function for the database
object, and while I was at it, I implemented compaction support
for it so we don't pin that proc.
@byroot byroot force-pushed the fix-busy-handler-compaction branch from b024d9b to e46e1d2 Compare January 9, 2024 15:54
@byroot
Copy link
Contributor Author

byroot commented Jan 9, 2024

Previous commit was failing on Truffle with:

TC_Database_Integration#test_trace:
RuntimeError: Neutered Exception Polyglot::ForeignException: External LLVMFunction rb_gc_mark_movable cannot be found.
    /home/runner/work/sqlite3-ruby/sqlite3-ruby/ext/sqlite3/database.c:16:in `database_mark'
    <internal:core> core/truffle/polyglot.rb:334:in `<unknown>'
    <internal:core> core/truffle/polyglot.rb:334:in `Truffle::Interop.execute'
    <internal:core> core/truffle/polyglot.rb:334:in `Polyglot::ExecutableTrait#call'
    /home/runner/.rubies/truffleruby-23.1.1/lib/truffle/truffle/cext.rb:1534:in `CExt#run_marker'
    /home/runner/.rubies/truffleruby-23.1.1/lib/truffle/truffle/cext.rb:1305:in `SQLite3::Database.__allocate__'
    /home/runner/work/sqlite3-ruby/sqlite3-ruby/test/test_integration.rb:5:in `TC_Database_Integration#setup'

Which is weird because looking at the Truffle source, that function very much seem to be defined, so I don't quite get it (FYI @eregon).

But overall, making that proc movable isn't that important, we're talking about one object per connection, so likely very few in each process, so not really worth checking for the function existence etc, I simply removed the compaction support and let the callback be pinned.

@byroot
Copy link
Contributor Author

byroot commented Jan 9, 2024

Now valgrind is failing, but the backtrace is inside sqlite itself, and I don't think I could possibly cause any of that with my changes 🤔

@eregon
Copy link
Contributor

eregon commented Jan 9, 2024

Re rb_gc_mark_movable it's implemented on truffleruby master and will be in 24.0 but is not in a release yet.
See https://github.com/oracle/truffleruby/blob/master/CHANGELOG.md

@byroot
Copy link
Contributor Author

byroot commented Jan 9, 2024

Ah! My bad, I saw it it the source from a fairly old commit and I assumed it was there. Thanks @eregon (and sorry for not looking more attentively).

@tenderlove
Copy link
Member

Storing VALUE self as context for rb_sqlite3_busy_handler is unsafe because as of Ruby 2.7, the GC compactor may move objects around which can lead to this reference pointing to either another random object or to garbage.

Ugh yep. This PR makes sense. I don't see how this is related to the valgrind test either, so I'm going to merge.

@tenderlove tenderlove merged commit dfa2c8b into sparklemotion:main Jan 9, 2024
98 of 99 checks passed
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.

None yet

4 participants