Skip to content

Initial Ractor support #365

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
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Initial Ractor support #365

wants to merge 11 commits into from

Conversation

sandlerr
Copy link

Allows users to create / open databases, send queries, and process ResultSets inside a Ractor. However, does not allow passing the gem's domain objects through the Ractor communication channels.

See #299 .

@sandlerr sandlerr mentioned this pull request Dec 28, 2022
Open
flavorjones
flavorjones requested changes Dec 28, 2022
View reviewed changes
@sandlerr sandlerr requested a review from flavorjones December 29, 2022 12:50
@sandlerr
Copy link
Author

sandlerr commented Jan 5, 2023

Is there any outstanding feedback here? Thanks

tenderlove
tenderlove approved these changes Jan 3, 2024
View reviewed changes
Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

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

This seems fine to me. I haven't investigated everything we need to do to make C extensions Ractor safe but I think this is fine to start with!

@flavorjones
Copy link
Member

Great, I'll rebase and merge it today or tomorrow.

@flavorjones
Copy link
Member

flavorjones commented Jan 10, 2024 •
edited
Loading

So it seems like in Ruby 3.3, trying to pass a Database between ractors results in a Ractor::RemoteError being raised; but in earlier versions of Ruby it makes an implicit copy of the object. This means that the test_ractor_share_database test hangs.

I'm not sure if this is intentional behavior or not. @sandlerr or @tenderlove do you know?

Edit: to be explicit, the RemoteError is wrapping Ractor::Error: can not copy SQLite3::Database object.

@flavorjones
Copy link
Member

OK, that exception started to get raised in ruby/ruby@ce47ee00 as a fix, so I'm just going to adapt the test to the different behaviors before and after 3.3.0.

@flavorjones flavorjones force-pushed the sandlerr/ractor branch 2 times, most recently from 0f4db3d to 104ea23 Compare January 10, 2024 22:09
@tenderlove tenderlove mentioned this pull request Jan 10, 2024
Closed
19 tasks
@flavorjones
Copy link
Member

The stress test hangs under Ruby 3.3 with as few as 3 ractor writers, and it's not clear to me why.

@flavorjones flavorjones self-requested a review January 11, 2024 04:05
@tenderlove tenderlove self-assigned this Jan 24, 2024
@tenderlove
Merge branch 'main' into sandlerr/ractor
093c6cc 
* main: (215 commits)
  version bump to v2.6.0
  Database#enable_load_extension def only if extensions are available
  dep: update vendored sqlite to v3.49.1
  dep: update vendored sqlite to 3.49.0
  dep: update vendored sqlite to 3.48.0
  build(deps-dev): update rdoc requirement from 6.11.0 to 6.12.0
  dev: Add rake task `test:gdb`
  fix: tests pass on bigendian architecture
  dep: move style and docs deps into a separate group
  ci: test-gem-install should use `[` not `[[`
  test: skip database URI tests when using system libraries
  doc: update CHANGELOG
  dep: update rake-compiler-dock to v1.9.1
  build(deps-dev): update rdoc requirement from 6.10.0 to 6.11.0
  dep(doc): fixing psych installation on alpine
  dep(test): remove implicit dependence on benchmark gem
  build(deps-dev): update ruby_memcheck requirement from 3.0.0 to 3.0.1
  dep: update rake-compiler-dock to v1.8.0
  build(deps-dev): update rake-compiler requirement from 1.2.8 to 1.2.9
  ci: bump test image tag from 3.4-rc to 3.4
  ...
@tenderlove
Allow databases to be shareable but only in full-mutex mode
2b0fff5 
  https://www.sqlite.org/threadsafe.html

"Full mutex" mode is the only time when a SQLite database can be shared
between multiple threads.  Multi-thread mode only means that multiple
threads can have their own instances of a database object that point at
the same file, but multiple threads cannot access the object
concurrently.
@tenderlove
Merge branch 'main' into sandlerr/ractor
36af661 
* main:
  build(deps-dev): update rdoc requirement from 6.12.0 to 6.13.0
  ci: get upstream sqlite-head job green
  build(deps-dev): update minitest requirement from 5.25.4 to 5.25.5
  Add parameter checking for string value pragmas
  Simplify PRAGMA related constants
  Add some regression tests for setting pragmas
  Freeze strings and constants in pragmas
@tenderlove
Copy link
Member

I made some updates to this branch and included a RACTOR.md file that describes Ractor support. Also I removed the "Ractor smoke test" test because I think it's just testing the guarantees that the SQLite library makes and not testing our APIs.

@flavorjones please take a look at the RACTOR.md file, I think it covers most of the information behind Ractor support. I think this PR is good to merge for the most part.

@flavorjones
Copy link
Member

Rebased onto main

@flavorjones
Copy link
Member

flavorjones commented Mar 30, 2025 •
edited
Loading

Looks like the FreeBSD build is failing, not sure if it's related or not:

/home/runner/work/sqlite3-ruby/sqlite3-ruby/test/test_collation.rb:64: [BUG] pthread_mutex_lock: Resource deadlock avoided (EDEADLK)
  ruby 3.2.6 (2024-10-30 revision 63aeb018eb) [amd64-freebsd14]

I'll re-run it in case it's a flake.

@flavorjones
Copy link
Member

@tenderlove It looks like there are still problems with this branch. Looks like a few other tests have deadlocked or are hanging, in addition to FreeBSD.

@flavorjones
Copy link
Member

I may have rebased this incorrectly ... not sure what's going on but when I rebase RACTOR.md disappears. I'll come back to it later today.

@tenderlove
Copy link
Member

I may have rebased this incorrectly ... not sure what's going on but when I rebase RACTOR.md disappears. I'll come back to it later today.

I think I accidentally committed to the merge commit. I can fix up the commits (but it'll be a while)

@tenderlove
Copy link
Member

@tenderlove It looks like there are still problems with this branch. Looks like a few other tests have deadlocked or are hanging, in addition to FreeBSD.

I was going to ask if we do any forking in the tests, but we have the fork tracker, so clearly we do. IIRC there are problems with Ractors and forking such that we can't create a Ractor and fork in the same process (at the moment, it's a bug we're trying to fix). I think we could either break the Ractor tests in to a different suite (maybe something like rake test:ractors) or maybe run them in a sub-process?

cc @jhawthorn

@flavorjones
Copy link
Member

flavorjones commented Mar 30, 2025 •
edited
Loading

@tenderlove I've restored your branch at 433c2d3, and I was definitely having problems because git rebase drops merge commits by default.

I would really prefer this branch to be rebased cleanly before we merge it, though.

Letting CI run again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenderlove tenderlove tenderlove approved these changes

@flavorjones flavorjones Awaiting requested review from flavorjones

Assignees

@tenderlove tenderlove

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sandlerr @flavorjones @tenderlove