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

Gem is not threadsafe #391

Closed
irminsul opened this issue Jul 11, 2023 · 4 comments · Fixed by #497
Closed

Gem is not threadsafe #391

irminsul opened this issue Jul 11, 2023 · 4 comments · Fixed by #497
Assignees
Milestone

Comments

@irminsul
Copy link

irminsul commented Jul 11, 2023

After spending quite some time chasing an obscure bug in one of my employer's applications, I thought I'd share my experiences here so that other people might benefit from it.

Our code contained a singleton that used a prepared statement to query a SQLite DB in read-only mode. This led to strange problems like empty result sets for queries that were guaranteed to return at least one row.

For us, these issues only manifested when running the app in production with Puma (a multi-threaded web server).

Here is a simple reproducer:

#!/usr/bin/env ruby

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'sqlite3', '~> 1.6.3'
end

# Ensure that we have a thread-safe SQLite library for this test
raise 'SQLite3 is not threadsafe' unless SQLite3.threadsafe?

db = SQLite3::Database.new ':memory:'
statement = db.prepare('SELECT :foo')

threads = (0...5).map do |i|
  Thread.new do
    pp "Thread #{i}: starting"

    1000.times do
      pp "Thread #{i}: lookup"

      result = statement.execute!(foo: 1)

      "Thread #{i}: error" if result.nil? || result.empty?
    end

    pp "Thread #{i}: done"
  end
end

threads.each(&:join)

This will eventually result in an exception like this:

#<Thread:0x00007ff4471c2c10 ./bug.rb:17 run> terminated with exception (report_on_exception is true):
/home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:39:in `bind_param': bad parameter or other API misuse (SQLite3::MisuseException)
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:39:in `block (2 levels) in bind_params'
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:39:in `each'
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:39:in `block in bind_params'
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:37:in `each'
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:37:in `bind_params'
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:64:in `execute'
        from /home/mz/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/sqlite3-1.6.3-x86_64-linux/lib/sqlite3/statement.rb:88:in `execute!'
        from ./bug.rb:23:in `block (3 levels) in <main>'
        from ./bug.rb:20:in `times'
        from ./bug.rb:20:in `block (2 levels) in <main>'

In hindsight, this behaviour is not surprising me at all.

My concern is that it is very easy to introduce this kind of concurrency bug, even if the application code itself does not do any explicit threading.

Judging by the discussions in some other issues (e.g. #217, #287), I think that the README should contain a prominent warning about using this gem in multi-threaded code; even if libsqlite3.so itself has been compiled with -DSQLITE_THREADSAFE=2.

@flavorjones
Copy link
Member

Thanks for opening this issue. I'd like to spend some time to understand more deeply why this is happening.

@ioquatix
Copy link

I believe if you have separate instances of the DB, one per thread, it will work correctly. To achieve this easily, you can use the thread-local gem. https://github.com/socketry/thread-local

@dbackeus
Copy link

dbackeus commented Dec 20, 2023

Should also work when using a connection pool (eg. ActiveRecord::Base.connection_pool) properly right?

Ie. as long as no two threads use the same connection concurrently I'm assuming things will be fine?

@tenderlove tenderlove self-assigned this Jan 24, 2024
@tenderlove
Copy link
Member

I believe if you have separate instances of the DB, one per thread, it will work correctly. To achieve this easily, you can use the thread-local gem. https://github.com/socketry/thread-local

I don't think this is the problem OP is trying to demo. Separate instances of an in-memory database may not make sense. I think the database connection itself is threadsafe and should be fine to share, but the script that OP presented is sharing a prepared statement among threads.

I've reduced the repro to this:

require "sqlite3"

# Ensure that we have a thread-safe SQLite library for this test
raise 'SQLite3 is not threadsafe' unless SQLite3.threadsafe?

db = SQLite3::Database.new ':memory:'
statement = db.prepare('SELECT :foo')

# Thread 1
statement.reset!

# Thread 2
statement.reset!

# Thread 1
statement.bind_params(foo: 1)
statement.step

# Thread 2
statement.bind_params(foo: 2) # exception

Prepared statements are basically mutable (you can bind parameters, step through results, etc) so I'm not really clear on how to ensure their entire API is thread safe. For example, we on the sqlite3-ruby team cannot make the script I posted above thread safe for the user. However, we could synchronize "one shot" APIs like execute!.

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 a pull request may close this issue.

5 participants