diff --git a/.editorconfig b/.editorconfig index d11eb878..d97646ac 100644 --- a/.editorconfig +++ b/.editorconfig @@ -1,15 +1,12 @@ root = true -[*.c,*.h] +[*] +indent_size = 2 + +[*.{c,h}] end_of_line = lf indent_size = 4 indent_style = space insert_final_newline = true tab_width = 8 trim_trailing_whitespace = true - -[*.rb,Rakefile,*.rake,*.gemspec] -indent_size = 2 - -[*.yml] -indent_size = 2 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ac6e3ac..4155f77d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ jobs: fail-fast: false matrix: os: [ubuntu, macos, windows] - ruby: ["3.3", "3.2", "3.1", "3.0"] + ruby: ["3.3", "3.2", "3.1"] syslib: [enable, disable] include: # additional compilation flags for homebrew @@ -86,10 +86,10 @@ jobs: # reported at https://github.com/sparklemotion/sqlite3-ruby/issues/354 # TODO remove once https://github.com/flavorjones/mini_portile/issues/118 is fixed needs: basic - name: "fedora:35" + name: "fedora:40" runs-on: ubuntu-latest container: - image: fedora:35 + image: fedora:40 steps: - run: | dnf group install -y "C Development Tools and Libraries" @@ -122,7 +122,7 @@ jobs: fail-fast: false matrix: os: [ubuntu, macos, windows] - ruby: ["3.3", "3.0"] # oldest and newest + ruby: ["3.3", "3.1"] # oldest and newest include: - { os: windows, ruby: mingw } - { os: windows, ruby: mswin } @@ -213,7 +213,7 @@ jobs: fail-fast: false matrix: os: [ubuntu, macos, windows] - ruby: ["3.3", "3.2", "3.1", "3.0"] + ruby: ["3.3", "3.2", "3.1"] syslib: [enable, disable] include: # additional compilation flags for homebrew @@ -246,7 +246,6 @@ jobs: - arm-linux-musl - arm64-darwin - x64-mingw-ucrt - - x64-mingw32 - x86-linux-gnu - x86-linux-musl - x86_64-darwin @@ -284,7 +283,7 @@ jobs: - x86-linux-musl - x86_64-linux-gnu - x86_64-linux-musl - ruby: ["3.3", "3.2", "3.1", "3.0"] + ruby: ["3.3", "3.2", "3.1"] include: # declare docker image for each platform - { platform: aarch64-linux-musl, docker_tag: "-alpine", bootstrap: "apk add build-base &&" } @@ -322,15 +321,12 @@ jobs: fail-fast: false matrix: os: [windows-latest, macos-13, macos-14] - ruby: ["3.3", "3.2", "3.1", "3.0"] + ruby: ["3.3", "3.2", "3.1"] include: - os: macos-13 platform: x86_64-darwin - os: macos-14 platform: arm64-darwin - - os: windows-latest - ruby: "3.0" - platform: x64-mingw32 - os: windows-latest ruby: "3.1" platform: x64-mingw-ucrt @@ -359,7 +355,6 @@ jobs: fail-fast: false matrix: include: - - { ruby: "3.0", flavor: "alpine" } - { ruby: "3.1", flavor: "alpine3.18" } - { ruby: "3.1", flavor: "alpine3.19" } - { ruby: "3.2", flavor: "alpine3.18" } @@ -376,6 +371,4 @@ jobs: name: cruby-x86_64-linux-musl-gem path: gems - run: apk add build-base - - if: matrix.ruby == '3.0' # https://github.com/rake-compiler/rake-compiler/pull/236 - run: gem update --system - run: ./bin/test-gem-install ./gems diff --git a/.rubocop.yml b/.rubocop.yml index faeea9f9..62458687 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -13,7 +13,7 @@ inherit_gem: AllCops: SuggestExtensions: false - TargetRubyVersion: 3.0 + TargetRubyVersion: 3.1 Naming/InclusiveLanguage: Enabled: true diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a7e650..90ca0ad6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,35 @@ # sqlite3-ruby Changelog +## 2.1.0 / 2024-09-24 + +### Ruby + +- This release drops support for Ruby 3.0. [#563] @flavorjones + + +### Fork safety improvements + +Sqlite itself is [not fork-safe](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_). Specifically, writing in a child process to a database connection that was created in the parent process may corrupt the database file. To mitigate this risk, sqlite3-ruby has implemented the following changes: + +- All open writable database connections carried across a `fork()` will immediately be closed in the child process to mitigate the risk of corrupting the database file. +- These connections will be incompletely closed ("discarded") which will result in a one-time memory leak in the child process. + +If it's at all possible, we strongly recommend that you close writable database connections in the parent before forking. If absolutely necessary (and you know what you're doing), you may suppress the fork safety warnings by calling `SQLite3::ForkSafety.suppress_warnings!`. + +See the README's "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558, #565, #566] @flavorjones + + +### Improved + +- Use `sqlite3_close_v2` to close databases in a deferred manner if there are unclosed prepared statements. Previously closing a database while statements were open resulted in a `BusyException`. See https://www.sqlite.org/c3ref/close.html for more context. [#557] @flavorjones +- When setting a Database `busy_handler`, fire the write barrier to prevent potential crashes during the GC mark phase. [#556] @jhawthorn + + +### Documentation + +- The `FAQ.md` has been updated to fix some inaccuracies. [#562] @rickhull + + ## 2.0.4 / 2024-08-13 ### Dependencies diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 97f06197..cccea932 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,24 +7,24 @@ This doc is a short introduction on how to modify and maintain the sqlite3-ruby ## Architecture notes +### Decision record + +As of 2024-09, we're starting to keep some architecture decisions in the subdirectory `/adr`, so +please look there for additional information. + ### Garbage collection All statements keep pointers back to their respective database connections. The `@connection` instance variable on the `Statement` handle keeps the database -connection alive. Memory allocated for a statement handler will be freed in -two cases: - -1. `#close` is called on the statement -2. The `SQLite3::Database` object gets garbage collected +connection alive. -We can't free the memory for the statement in the garbage collection function -for the statement handler. The reason is because there exists a race -condition. We cannot guarantee the order in which objects will be garbage -collected. So, it is possible that a connection and a statement are up for -garbage collection. If the database connection were to be free'd before the -statement, then boom. Instead we'll be conservative and free unclosed -statements when the connection is terminated. +We use `sqlite3_close_v2` in `Database#close` since v2.1.0 which defers _actually_ closing the +connection and freeing the underlying memory until all open statments are closed; though the +`Database` object will immediately behave as though it's been fully closed. If a Database is not +explicitly closed, it will be closed when it is GCed. +`Statement#close` finalizes the underlying statement. If a Statement is not explicitly closed, it +will be closed/finalized when it is GCed. ## Building gems diff --git a/FAQ.md b/FAQ.md index 63d7e2f7..eb43875b 100644 --- a/FAQ.md +++ b/FAQ.md @@ -207,48 +207,46 @@ Or do a `Database#prepare` to get the `Statement`, and then use either stmt.bind_params( "value", "name" => "bob" ) ``` -## How do I discover metadata about a query? +## How do I discover metadata about a query result? -If you ever want to know the names or types of the columns in a result -set, you can do it in several ways. +IMPORTANT: `Database#execute` returns an Array of Array of Strings +which will have no metadata about the query or the result, such +as column names. -The first way is to ask the row object itself. Each row will have a -property "fields" that returns an array of the column names. The row -will also have a property "types" that returns an array of the column -types: +There are 2 main sources of query metadata: - -```ruby - rows = db.execute( "select * from table" ) - p rows[0].fields - p rows[0].types -``` +* `Statement` +* `ResultSet` -Obviously, this approach requires you to execute a statement that actually -returns data. If you don't know if the statement will return any rows, but -you still need the metadata, you can use `Database#query` and ask the -`ResultSet` object itself: +You can get a `Statement` via `Database#prepare`, and you can get +a `ResultSet` via `Statement#execute` or `Database#query`. ```ruby - db.query( "select * from table" ) do |result| - p result.columns - p result.types - ... - end -``` - - -Lastly, you can use `Database#prepare` and ask the `Statement` object what -the metadata are: - - -```ruby - stmt = db.prepare( "select * from table" ) - p stmt.columns - p stmt.types +sql = 'select * from table' + +# No metadata +rows = db.execute(sql) +rows.class # => Array, no metadata +rows.first.class # => Array, no metadata +rows.first.first.class #=> String, no metadata + +# Statement has metadata +stmt = db.prepare(sql) +stmt.columns # => [ ... ] +stmt.types # => [ ... ] + +# ResultSet has metadata +results = stmt.execute +results.columns # => [ ... ] +results.types # => [ ... ] + +# ResultSet has metadata +results = db.query(sql) +results.columns # => [ ... ] +results.types # => [ ... ] ``` ## I'd like the rows to be indexible by column name. @@ -273,7 +271,18 @@ is unavailable on the row, although the "types" property remains.) ``` -The other way is to use Ara Howard's +A more granular way to do this is via `ResultSet#next_hash` or +`ResultSet#each_hash`. + + +```ruby + results = db.query( "select * from table" ) + row = results.next_hash + p row['column1'] +``` + + +Another way is to use Ara Howard's [`ArrayFields`](http://rubyforge.org/projects/arrayfields) module. Just `require "arrayfields"`, and all of your rows will be indexable by column name, even though they are still arrays! diff --git a/Gemfile b/Gemfile index 59ae440b..a56fee0b 100644 --- a/Gemfile +++ b/Gemfile @@ -3,7 +3,7 @@ source "https://rubygems.org" gemspec group :development do - gem "minitest", "5.24.1" + gem "minitest", "5.25.1" gem "rake-compiler", "1.2.7" gem "rake-compiler-dock", "1.5.2" diff --git a/INSTALLATION.md b/INSTALLATION.md index 50e5055c..e3dd5f3f 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -14,15 +14,13 @@ In v2.0.0 and later, native (precompiled) gems are available for recent Ruby ver - `arm-linux-gnu` (requires: glibc >= 2.29) - `arm-linux-musl` - `arm64-darwin` -- `x64-mingw32` / `x64-mingw-ucrt` +- `x64-mingw-ucrt` - `x86-linux-gnu` (requires: glibc >= 2.17) - `x86-linux-musl` - `x86_64-darwin` - `x86_64-linux-gnu` (requires: glibc >= 2.17) - `x86_64-linux-musl` -⚠ Ruby 3.0 linux users must use Rubygems >= 3.3.22 in order to use these gems. - ⚠ Musl linux users should update to Bundler >= 2.5.6 to avoid https://github.com/rubygems/rubygems/issues/7432 If you are using one of these Ruby versions on one of these platforms, the native gem is the recommended way to install sqlite3-ruby. diff --git a/README.md b/README.md index be332c20..b83dcf7e 100644 --- a/README.md +++ b/README.md @@ -148,6 +148,23 @@ It is generally recommended that if applications want to share a database among threads, they _only_ share the database instance object. Other objects are fine to share, but may require manual locking for thread safety. + +## Fork Safety + +[Sqlite is not fork +safe](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_) +and instructs users to not carry an open writable database connection across a `fork()`. Using an inherited +connection in the child may corrupt your database, leak memory, or cause other undefined behavior. + +To help protect users of this gem from accidental corruption due to this lack of fork safety, the gem will immediately close any open writable databases in the child after a fork. Discarding writable +connections in the child will incur a small one-time memory leak per connection, but that's +preferable to potentially corrupting your database. + +Whenever possible, close writable connections in the parent before forking. If absolutely necessary (and you know what you're doing), you may suppress the fork safety warnings by calling `SQLite3::ForkSafety.suppress_warnings!`. + +See [./adr/2024-09-fork-safety.md](./adr/2024-09-fork-safety.md) for more information and context. + + ## Support ### Installation or database extensions diff --git a/adr/2024-09-fork-safety.md b/adr/2024-09-fork-safety.md new file mode 100644 index 00000000..b5b26c37 --- /dev/null +++ b/adr/2024-09-fork-safety.md @@ -0,0 +1,72 @@ + +# 2024-09 Automatically close database connections when carried across fork() + +## Status + +Accepted, but we can revisit more complex solutions if we learn something that indicates that effort is worth it. + + +## Context + +In August 2024, Andy Croll opened an issue[^issue] describing sqlite file corruption related to solid queue. After investigation, we were able to reproduce corruption under certain circumstances when forking a process with open sqlite databases.[^repro] + +SQLite is known to not be fork-safe[^howto], so this was not entirely surprising though it was the first time your author had personally seen corruption in the wild. The corruption became much more likely after the sqlite3-ruby gem improved its memory management with respect to open statements[^gemleak] in v2.0.0. + +Advice from upstream contributors[^advice] is, essentially: don't fork if you have open database connections. Or, if you have forked, don't call `sqlite3_close` on those connections and thereby leak some amount of memory in the child process. Neither of these options are ideal, see below. + + +## Decisions + +1. Open writable database connections carried across a `fork()` will automatically be closed in the child process to mitigate the risk of corrupting the database file. +2. These connections will be incompletely closed ("discarded") which will result in a one-time memory leak in the child process. + +First, the gem will register an "after fork" handler via `Process._fork` that will close any open writable database connections in the child process. This is a best-effort attempt to avoid corruption, but it is not guaranteed to prevent corruption in all cases. Any connections closed by this handler will also emit a warning to let users know what's happening. + +Second, the sqlite3-ruby gem will store the ID of the process that opened each database connection. If, when a writable database is closed (either explicitly with `Database#close` or implicitly via GC or after-fork callback) the current process ID is different from the original process, then we "discard" the connection. + +"Discard" here means: + +- `sqlite3_close_v2` is not called on the database, because it is unsafe to do so per sqlite instructions[^howto]. + - Open file descriptors associated with the database are closed. + - Any memory that can be freed safely is recovered. + - But some memory will be lost permanently (a one-time "memory leak"). +- The `Database` object acts "closed", including returning `true` from `#closed?`. +- Related `Statement` objects are rendered unusable and will raise an exception if used. + +Note that readonly databases are being treated as "fork safe" and are not affected by these changes. + + +## Consequences + +The positive consequence is that we remove a potential cause of database corruption for applications that fork with active sqlite database connections. + +The negative consequence is that, for each discarded connection, some memory will be permanently lost (leaked) in the child process. We consider this to be an acceptable tradeoff given the risk of data loss. + + +## Alternatives considered. + +### 1. Require applications to close database connections before forking. + +This is the advice[^advice] given by the upstream maintainers of sqlite, and so was the first thing we tried to implement in Rails in [rails/rails#52931](https://github.com/rails/rails/pull/52931)[^before_fork]. That first simple implementation was not thread safe, however, and in order to make it thread-safe it would be necessary to pause all sqlite database activity, close the open connections, and then fork. At least one Rails core team member was not happy that this would interfere with database connections in the parent, and the complexity of a thread-safe solution seemed high, so this work was paused. + +### 2. Memory arena + +Sqlite offers a configuration option to specify custom memory functions for malloc et al. It seems possible that the sqlite3-ruby gem could implement a custom arena that would be used by sqlite so that in a new process, after forking, all the memory underlying the sqlite Ruby objects could be discarded in a single operation. + +I think this approach is promising, but complex and risky. Sqlite is a complex library and uses shared memory in addition to the traditional heap. Would throwing away the heap memory (the arena) result in a segfault or other undefined behaviors or corruption? Determining the answer to that question feels expensive in and of itself, and any solution along these lines would not be supported by the sqlite authors. We can explore this space if the memory leak from discarded connections turns out to be a large source of pain. + + +## References + +- [Database connections carried across fork() will not be fully closed by flavorjones · Pull Request #558 · sparklemotion/sqlite3-ruby](https://github.com/sparklemotion/sqlite3-ruby/pull/558) + + +## Footnotes + +[^issue]: [SQLite queue database corruption · Issue #324 · rails/solid_queue](https://github.com/rails/solid_queue/issues/324) +[^repro]: [flavorjones/2024-09-13-sqlite-corruption: Temporary repo, reproduction of sqlite database corruption.](https://github.com/flavorjones/2024-09-13-sqlite-corruption) +[^howto]: [How To Corrupt An SQLite Database File: §2.6 Carrying an open database connection across a fork()](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_) +[^gemleak]: [Always call sqlite3_finalize in deallocate func by haileys · Pull Request #392 · sparklemotion/sqlite3-ruby](https://github.com/sparklemotion/sqlite3-ruby/pull/392) +[^advice]: [SQLite Forum: Correct way of carrying connections over forked processes](https://sqlite.org/forum/forumpost/1fa07728204567a0a136f442cb1c59e3117da96898b7fa3290b0063ae7f6f012) +[^before_fork]: [SQLite3Adapter: Ensure fork-safety by flavorjones · Pull Request #52931 · rails/rails](https://github.com/rails/rails/pull/52931#issuecomment-2351365601) +[^config]: [SQlite3 Configuration Options](https://www.sqlite.org/c3ref/c_config_covering_index_scan.html) diff --git a/bin/test-gem-file-contents b/bin/test-gem-file-contents index 284c4fea..8ae6bea7 100755 --- a/bin/test-gem-file-contents +++ b/bin/test-gem-file-contents @@ -65,22 +65,7 @@ Minitest::Reporters.use!([Minitest::Reporters::SpecReporter.new]) puts "Testing '#{gemfile}' (#{gemspec.platform})" describe File.basename(gemfile) do - let(:all_supported_ruby_versions) { - ["3.0", "3.1", "3.2", "3.3"] - } - let(:native_supported_ruby_versions) { ["3.0", "3.1", "3.2", "3.3"] } - let(:ucrt_supported_ruby_versions) { ["3.1", "3.2", "3.3"] } - let(:platform_supported_ruby_versions) do - if gemspec.platform.to_s == "x64-mingw-ucrt" - ucrt_supported_ruby_versions - elsif gemspec.platform.to_s == "x64-mingw32" - native_supported_ruby_versions - ucrt_supported_ruby_versions - elsif gemspec.platform.cpu - native_supported_ruby_versions - else - all_supported_ruby_versions - end - end + let(:supported_ruby_versions) { ["3.1", "3.2", "3.3"] } describe "setup" do it "gemfile contains some files" do @@ -147,7 +132,7 @@ describe File.basename(gemfile) do end it "sets required_ruby_version appropriately" do - all_supported_ruby_versions.each do |v| + supported_ruby_versions.each do |v| assert( gemspec.required_ruby_version.satisfied_by?(Gem::Version.new(v)), "required_ruby_version='#{gemspec.required_ruby_version}' should support ruby #{v}" @@ -181,7 +166,7 @@ describe File.basename(gemfile) do end it "contains expected shared library files " do - platform_supported_ruby_versions.each do |version| + supported_ruby_versions.each do |version| actual = gemfile_contents.find do |p| File.fnmatch?("lib/sqlite3/#{version}/sqlite3_native.{so,bundle}", p, File::FNM_EXTGLOB) end @@ -197,26 +182,19 @@ describe File.basename(gemfile) do File.fnmatch?("lib/sqlite3/**/*.{so,bundle}", p, File::FNM_EXTGLOB) end assert_equal( - platform_supported_ruby_versions.length, + supported_ruby_versions.length, actual.length, "did not expect extra shared library files" ) end it "sets required_ruby_version appropriately" do - unsupported_versions = all_supported_ruby_versions - platform_supported_ruby_versions - platform_supported_ruby_versions.each do |v| + supported_ruby_versions.each do |v| assert( gemspec.required_ruby_version.satisfied_by?(Gem::Version.new(v)), "required_ruby_version='#{gemspec.required_ruby_version}' should support ruby #{v}" ) end - unsupported_versions.each do |v| - refute( - gemspec.required_ruby_version.satisfied_by?(Gem::Version.new(v)), - "required_ruby_version='#{gemspec.required_ruby_version}' should not support ruby #{v}" - ) - end end it "does not set metadata for msys2" do diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index d61bf0ac..621dc7aa 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -12,6 +12,64 @@ VALUE cSqlite3Database; +/* See adr/2024-09-fork-safety.md */ +static void +discard_db(sqlite3RubyPtr ctx) +{ + sqlite3_file *sfile; + int status; + + // release as much heap memory as possible by deallocating non-essential memory + // allocations held by the database library. Memory used to cache database pages to + // improve performance is an example of non-essential memory. + // on my development machine, this reduces the lost memory from 152k to 69k. + sqlite3_db_release_memory(ctx->db); + + // release file descriptors +#ifdef HAVE_SQLITE3_DB_NAME + const char *db_name; + int j_db = 0; + while ((db_name = sqlite3_db_name(ctx->db, j_db)) != NULL) { + status = sqlite3_file_control(ctx->db, db_name, SQLITE_FCNTL_FILE_POINTER, &sfile); + if (status == 0 && sfile->pMethods != NULL) { + sfile->pMethods->xClose(sfile); + } + j_db++; + } +#else + status = sqlite3_file_control(ctx->db, NULL, SQLITE_FCNTL_FILE_POINTER, &sfile); + if (status == 0 && sfile->pMethods != NULL) { + sfile->pMethods->xClose(sfile); + } +#endif + + status = sqlite3_file_control(ctx->db, NULL, SQLITE_FCNTL_JOURNAL_POINTER, &sfile); + if (status == 0 && sfile->pMethods != NULL) { + sfile->pMethods->xClose(sfile); + } + + ctx->db = NULL; + ctx->flags |= SQLITE3_RB_DATABASE_DISCARDED; +} + +static void +close_or_discard_db(sqlite3RubyPtr ctx) +{ + if (ctx->db) { + int is_readonly = (ctx->flags & SQLITE3_RB_DATABASE_READONLY); + + if (is_readonly || ctx->owner == getpid()) { + // Ordinary close. + sqlite3_close_v2(ctx->db); + ctx->db = NULL; + } else { + // This is an open connection carried across a fork(). "Discard" it. + discard_db(ctx); + } + } +} + + static void database_mark(void *ctx) { @@ -22,11 +80,8 @@ database_mark(void *ctx) static void deallocate(void *ctx) { - sqlite3RubyPtr c = (sqlite3RubyPtr)ctx; - sqlite3 *db = c->db; - - if (db) { sqlite3_close(db); } - xfree(c); + close_or_discard_db((sqlite3RubyPtr)ctx); + xfree(ctx); } static size_t @@ -51,7 +106,9 @@ static VALUE allocate(VALUE klass) { sqlite3RubyPtr ctx; - return TypedData_Make_Struct(klass, sqlite3Ruby, &database_type, ctx); + VALUE object = TypedData_Make_Struct(klass, sqlite3Ruby, &database_type, ctx); + ctx->owner = getpid(); + return object; } static char * @@ -62,8 +119,6 @@ utf16_string_value_ptr(VALUE str) return RSTRING_PTR(str); } -static VALUE sqlite3_rb_close(VALUE self); - sqlite3RubyPtr sqlite3_database_unwrap(VALUE database) { @@ -77,6 +132,7 @@ rb_sqlite3_open_v2(VALUE self, VALUE file, VALUE mode, VALUE zvfs) { sqlite3RubyPtr ctx; int status; + int flags; TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); @@ -89,14 +145,18 @@ rb_sqlite3_open_v2(VALUE self, VALUE file, VALUE mode, VALUE zvfs) # endif #endif + flags = NUM2INT(mode); status = sqlite3_open_v2( StringValuePtr(file), &ctx->db, - NUM2INT(mode), + flags, NIL_P(zvfs) ? NULL : StringValuePtr(zvfs) ); - CHECK(ctx->db, status) + CHECK(ctx->db, status); + if (flags & SQLITE_OPEN_READONLY) { + ctx->flags |= SQLITE3_RB_DATABASE_READONLY; + } return self; } @@ -119,21 +179,38 @@ rb_sqlite3_disable_quirk_mode(VALUE self) #endif } -/* call-seq: db.close +/* + * Close the database and release all associated resources. * - * Closes this database. + * ⚠ Writable connections that are carried across a fork() are not completely + * closed. {Sqlite does not support forking}[https://www.sqlite.org/howtocorrupt.html], + * and fully closing a writable connection that has been carried across a fork may corrupt the + * database. Since it is an incomplete close, not all memory resources are freed, but this is safer + * than risking data loss. + * + * See rdoc-ref:adr/2024-09-fork-safety.md for more information on fork safety. */ static VALUE sqlite3_rb_close(VALUE self) { sqlite3RubyPtr ctx; - sqlite3 *db; TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); - db = ctx->db; - CHECK(db, sqlite3_close(ctx->db)); + close_or_discard_db(ctx); - ctx->db = NULL; + rb_iv_set(self, "-aggregators", Qnil); + + return self; +} + +/* private method, primarily for testing */ +static VALUE +sqlite3_rb_discard(VALUE self) +{ + sqlite3RubyPtr ctx; + TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); + + discard_db(ctx); rb_iv_set(self, "-aggregators", Qnil); @@ -246,7 +323,7 @@ busy_handler(int argc, VALUE *argv, VALUE self) rb_scan_args(argc, argv, "01", &block); if (NIL_P(block) && rb_block_given_p()) { block = rb_block_proc(); } - ctx->busy_handler = block; + RB_OBJ_WRITE(self, &ctx->busy_handler, block); status = sqlite3_busy_handler( ctx->db, @@ -869,6 +946,9 @@ rb_sqlite3_open16(VALUE self, VALUE file) #endif #endif + // sqlite3_open16 implicitly uses flags (SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE) + // see https://www.sqlite.org/capi3ref.html#sqlite3_open + // so we do not ever set SQLITE3_RB_DATABASE_READONLY in ctx->flags status = sqlite3_open16(utf16_string_value_ptr(file), &ctx->db); CHECK(ctx->db, status) @@ -889,6 +969,7 @@ init_sqlite3_database(void) rb_define_private_method(cSqlite3Database, "open16", rb_sqlite3_open16, 1); rb_define_method(cSqlite3Database, "collation", collation, 2); rb_define_method(cSqlite3Database, "close", sqlite3_rb_close, 0); + rb_define_private_method(cSqlite3Database, "discard", sqlite3_rb_discard, 0); rb_define_method(cSqlite3Database, "closed?", closed_p, 0); rb_define_method(cSqlite3Database, "total_changes", total_changes, 0); rb_define_method(cSqlite3Database, "trace", trace, -1); diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h index 3123f4fe..04124881 100644 --- a/ext/sqlite3/database.h +++ b/ext/sqlite3/database.h @@ -3,11 +3,17 @@ #include +/* bits in the `flags` field */ +#define SQLITE3_RB_DATABASE_READONLY 0x01 +#define SQLITE3_RB_DATABASE_DISCARDED 0x02 + struct _sqlite3Ruby { sqlite3 *db; VALUE busy_handler; int stmt_timeout; struct timespec stmt_deadline; + rb_pid_t owner; + int flags; }; typedef struct _sqlite3Ruby sqlite3Ruby; diff --git a/ext/sqlite3/extconf.rb b/ext/sqlite3/extconf.rb index c648d9e9..021b3304 100644 --- a/ext/sqlite3/extconf.rb +++ b/ext/sqlite3/extconf.rb @@ -131,6 +131,8 @@ def configure_extension end have_func("sqlite3_prepare_v2") + have_func("sqlite3_db_name", "sqlite3.h") # v3.39.0 + have_type("sqlite3_int64", "sqlite3.h") have_type("sqlite3_uint64", "sqlite3.h") end diff --git a/ext/sqlite3/statement.c b/ext/sqlite3/statement.c index cb65efb7..705b7679 100644 --- a/ext/sqlite3/statement.c +++ b/ext/sqlite3/statement.c @@ -1,9 +1,13 @@ #include #define REQUIRE_OPEN_STMT(_ctxt) \ - if(!_ctxt->st) \ + if (!_ctxt->st) \ rb_raise(rb_path2class("SQLite3::Exception"), "cannot use a closed statement"); +#define REQUIRE_LIVE_DB(_ctxt) \ + if (_ctxt->db->flags & SQLITE3_RB_DATABASE_DISCARDED) \ + rb_raise(rb_path2class("SQLite3::Exception"), "cannot use a statement associated with a discarded database"); + VALUE cSqlite3Statement; static void @@ -57,6 +61,11 @@ prepare(VALUE self, VALUE db, VALUE sql) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + /* Dereferencing a pointer to the database struct will be faster than accessing it through the + * instance variable @connection. The struct pointer is guaranteed to be live because instance + * variable will keep it from being GCed. */ + ctx->db = db_ctx; + #ifdef HAVE_SQLITE3_PREPARE_V2 status = sqlite3_prepare_v2( #else @@ -121,6 +130,7 @@ step(VALUE self) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); if (ctx->done_p) { return Qnil; } @@ -216,6 +226,8 @@ bind_param(VALUE self, VALUE key, VALUE value) int index; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); switch (TYPE(key)) { @@ -308,6 +320,8 @@ reset_bang(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); sqlite3_reset(ctx->st); @@ -328,6 +342,8 @@ clear_bindings_bang(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); sqlite3_clear_bindings(ctx->st); @@ -360,6 +376,8 @@ column_count(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_column_count(ctx->st)); @@ -391,6 +409,8 @@ column_name(VALUE self, VALUE index) const char *name; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); name = sqlite3_column_name(ctx->st, (int)NUM2INT(index)); @@ -414,6 +434,8 @@ column_decltype(VALUE self, VALUE index) const char *name; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); name = sqlite3_column_decltype(ctx->st, (int)NUM2INT(index)); @@ -431,6 +453,8 @@ bind_parameter_count(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_bind_parameter_count(ctx->st)); @@ -538,7 +562,10 @@ stats_as_hash(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); + VALUE arg = rb_hash_new(); stmt_stat_internal(arg, ctx->st); @@ -554,6 +581,8 @@ stat_for(VALUE self, VALUE key) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); if (SYMBOL_P(key)) { @@ -574,6 +603,8 @@ memused(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_stmt_status(ctx->st, SQLITE_STMTSTATUS_MEMUSED, 0)); @@ -591,6 +622,8 @@ database_name(VALUE self, VALUE index) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return SQLITE3_UTF8_STR_NEW2( @@ -608,6 +641,8 @@ get_sql(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return rb_obj_freeze(SQLITE3_UTF8_STR_NEW2(sqlite3_sql(ctx->st))); @@ -626,6 +661,8 @@ get_expanded_sql(VALUE self) VALUE rb_expanded_sql; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); expanded_sql = sqlite3_expanded_sql(ctx->st); diff --git a/ext/sqlite3/statement.h b/ext/sqlite3/statement.h index d5dd343f..faae92b2 100644 --- a/ext/sqlite3/statement.h +++ b/ext/sqlite3/statement.h @@ -5,6 +5,7 @@ struct _sqlite3StmtRuby { sqlite3_stmt *st; + sqlite3Ruby *db; int done_p; }; diff --git a/lib/sqlite3/database.rb b/lib/sqlite3/database.rb index 39ace568..1cf9e62e 100644 --- a/lib/sqlite3/database.rb +++ b/lib/sqlite3/database.rb @@ -5,6 +5,7 @@ require "sqlite3/pragmas" require "sqlite3/statement" require "sqlite3/value" +require "sqlite3/fork_safety" module SQLite3 # The Database class encapsulates a single connection to a SQLite3 database. @@ -127,7 +128,6 @@ def initialize file, options = {}, zvfs = nil @tracefunc = nil @authorizer = nil - @busy_handler = nil @progress_handler = nil @collations = {} @functions = {} @@ -135,6 +135,8 @@ def initialize file, options = {}, zvfs = nil @readonly = mode & Constants::Open::READONLY != 0 @default_transaction_mode = options[:default_transaction_mode] || :deferred + ForkSafety.track(self) + if block_given? begin yield self diff --git a/lib/sqlite3/fork_safety.rb b/lib/sqlite3/fork_safety.rb new file mode 100644 index 00000000..69cf6ac3 --- /dev/null +++ b/lib/sqlite3/fork_safety.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require "weakref" + +# based on Rails's active_support/fork_tracker.rb +module SQLite3 + module ForkSafety + module CoreExt + def _fork + pid = super + if pid == 0 + ForkSafety.discard + end + pid + end + end + + @databases = [] + @mutex = Mutex.new + @suppress = false + + class << self + def hook! + ::Process.singleton_class.prepend(CoreExt) + end + + def track(database) + @mutex.synchronize do + @databases << WeakRef.new(database) + end + end + + def discard + warned = @suppress + @databases.each do |db| + next unless db.weakref_alive? + + unless db.closed? || db.readonly? + unless warned + # If you are here, you may want to read + # https://github.com/sparklemotion/sqlite3-ruby/pull/558 + warn("Writable sqlite database connection(s) were inherited from a forked process. " \ + "This is unsafe and the connections are being closed to prevent possible data " \ + "corruption. Please close writable sqlite database connections before forking.", + uplevel: 0) + warned = true + end + db.close + end + end + @databases.clear + end + + # Call to suppress the fork-related warnings. + def suppress_warnings! + @suppress = true + end + end + end +end + +SQLite3::ForkSafety.hook! diff --git a/lib/sqlite3/version.rb b/lib/sqlite3/version.rb index 6ad1f0f0..21b0c51c 100644 --- a/lib/sqlite3/version.rb +++ b/lib/sqlite3/version.rb @@ -1,3 +1,3 @@ module SQLite3 - VERSION = "2.0.4" + VERSION = "2.1.0" end diff --git a/rakelib/native.rake b/rakelib/native.rake index ff9177a8..4d29758e 100644 --- a/rakelib/native.rake +++ b/rakelib/native.rake @@ -6,7 +6,7 @@ require "rake/extensiontask" require "rake_compiler_dock" require "yaml" -cross_rubies = ["3.3.0", "3.2.0", "3.1.0", "3.0.0"] +cross_rubies = ["3.3.0", "3.2.0", "3.1.0"] cross_platforms = [ "aarch64-linux-gnu", "aarch64-linux-musl", @@ -14,7 +14,6 @@ cross_platforms = [ "arm-linux-musl", "arm64-darwin", "x64-mingw-ucrt", - "x64-mingw32", "x86-linux-gnu", "x86-linux-musl", "x86_64-darwin", diff --git a/sqlite3.gemspec b/sqlite3.gemspec index 57f5d61b..8283fbed 100644 --- a/sqlite3.gemspec +++ b/sqlite3.gemspec @@ -18,7 +18,7 @@ Gem::Specification.new do |s| s.licenses = ["BSD-3-Clause"] - s.required_ruby_version = Gem::Requirement.new(">= 3.0") + s.required_ruby_version = Gem::Requirement.new(">= 3.1") s.homepage = "https://github.com/sparklemotion/sqlite3-ruby" s.metadata = { @@ -62,6 +62,7 @@ Gem::Specification.new do |s| "lib/sqlite3/constants.rb", "lib/sqlite3/database.rb", "lib/sqlite3/errors.rb", + "lib/sqlite3/fork_safety.rb", "lib/sqlite3/pragmas.rb", "lib/sqlite3/resultset.rb", "lib/sqlite3/statement.rb", diff --git a/test/helper.rb b/test/helper.rb index 81b225cb..9f159247 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -1,10 +1,6 @@ require "sqlite3" require "minitest/autorun" -if ENV["GITHUB_ACTIONS"] == "true" || ENV["CI"] - $VERBOSE = nil -end - puts "info: ruby version: #{RUBY_DESCRIPTION}" puts "info: gem version: #{SQLite3::VERSION}" puts "info: sqlite version: #{SQLite3::SQLITE_VERSION}/#{SQLite3::SQLITE_LOADED_VERSION}" @@ -20,5 +16,10 @@ class TestCase < Minitest::Test def assert_nothing_raised yield end + + def i_am_running_in_valgrind + # https://stackoverflow.com/questions/365458/how-can-i-detect-if-a-program-is-running-from-within-valgrind/62364698#62364698 + ENV["LD_PRELOAD"] =~ /valgrind|vgpreload/ + end end end diff --git a/test/test_database.rb b/test/test_database.rb index 6662eab9..19723478 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -611,7 +611,7 @@ def call action, a, b, c, d def test_close_with_open_statements s = @db.prepare("select 'foo'") - assert_raises(SQLite3::BusyException) do + assert_nothing_raised do # formerly raised SQLite3::BusyException @db.close end ensure @@ -664,7 +664,7 @@ def test_load_extension_error def test_raw_float_infinity # https://github.com/sparklemotion/sqlite3-ruby/issues/396 - skip if SQLite3::SQLITE_LOADED_VERSION >= "3.43.0" + skip if SQLite3::SQLITE_LOADED_VERSION == "3.43.0" db = SQLite3::Database.new ":memory:" db.execute("create table foo (temperature float)") diff --git a/test/test_discarding.rb b/test/test_discarding.rb new file mode 100644 index 00000000..5877c9a4 --- /dev/null +++ b/test/test_discarding.rb @@ -0,0 +1,172 @@ +require_relative "helper" + +module SQLite3 + class TestDiscardDatabase < SQLite3::TestCase + DBPATH = "test.db" + + def setup + FileUtils.rm_f(DBPATH) + super + end + + def teardown + super + FileUtils.rm_f(DBPATH) + end + + def in_a_forked_process + @read, @write = IO.pipe + old_stderr, $stderr = $stderr, StringIO.new + + Process.fork do + @read.close + begin + yield @write + rescue => e + old_stderr.write("child exception: #{e.message}") + end + @write.write($stderr.string) + @write.close + exit! + end + + $stderr = old_stderr + @write.close + *@results = *@read.readlines + @read.close + end + + def test_fork_discards_an_open_readwrite_connection + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + db = SQLite3::Database.new(DBPATH) + + in_a_forked_process do |write| + write.write(db.closed? ? "ok\n" : "fail\n") + end + + assertion, *stderr = *@results + + assert_equal("ok", assertion.chomp, "closed? did not return true") + assert_equal(1, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + assert_match( + /warning: Writable sqlite database connection\(s\) were inherited from a forked process/, + stderr.first, + "expected warning was not emitted" + ) + ensure + db&.close + end + end + + def test_fork_does_not_discard_closed_connections + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + db = SQLite3::Database.new(DBPATH) + db.close + + in_a_forked_process do |write| + write.write(db.closed? ? "ok\n" : "fail\n") + write.write($stderr.string) # should be empty write, no warnings emitted + write.write("done\n") + end + + assertion, *rest = *@results + + assert_equal("ok", assertion.chomp, "closed? did not return true") + assert_equal(1, rest.count, "unexpected output on stderr: #{rest.inspect}") + assert_equal("done", rest.first.chomp, "unexpected output on stderr: #{rest.inspect}") + ensure + db&.close + end + end + + def test_fork_does_not_discard_readonly_connections + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + SQLite3::Database.open(DBPATH) do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new(DBPATH, readonly: true) + + in_a_forked_process do |write| + write.write(db.closed? ? "fail\n" : "ok\n") # should be open and readable + write.write((db.execute("select * from foo") == [[1]]) ? "ok\n" : "fail\n") + write.write($stderr.string) # should be an empty write, no warnings emitted + write.write("done\n") + end + + assertion1, assertion2, *rest = *@results + + assert_equal("ok", assertion1.chomp, "closed? did not return false") + assert_equal("ok", assertion2.chomp, "could not read from database") + assert_equal(1, rest.count, "unexpected output on stderr: #{rest.inspect}") + assert_equal("done", rest.first.chomp, "unexpected output on stderr: #{rest.inspect}") + ensure + db&.close + end + end + + def test_close_does_not_discard_readonly_connections + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + SQLite3::Database.open(DBPATH) do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new(DBPATH, readonly: true) + + in_a_forked_process do |write| + write.write(db.closed? ? "fail\n" : "ok\n") # should be open and readable + db.close + write.write($stderr.string) # should be an empty write, no warnings emitted + write.write("done\n") + end + + assertion, *rest = *@results + + assert_equal("ok", assertion.chomp, "closed? did not return false") + assert_equal(1, rest.count, "unexpected output on stderr: #{rest.inspect}") + assert_equal("done", rest.first.chomp, "unexpected output on stderr: #{rest.inspect}") + ensure + db&.close + end + end + + def test_a_discarded_connection_with_statements + skip("discard leaks memory") if i_am_running_in_valgrind + + begin + db = SQLite3::Database.new(DBPATH) + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + stmt = db.prepare("select * from foo") + + db.send(:discard) + + e = assert_raises(SQLite3::Exception) { stmt.execute } + assert_match(/cannot use a statement associated with a discarded database/, e.message) + + assert_nothing_raised { stmt.close } + assert_predicate(stmt, :closed?) + ensure + db&.close + end + end + end +end diff --git a/test/test_resource_cleanup.rb b/test/test_resource_cleanup.rb index fa23a0ac..51a4b2cf 100644 --- a/test/test_resource_cleanup.rb +++ b/test/test_resource_cleanup.rb @@ -17,11 +17,31 @@ def test_cleanup_unclosed_statement_object end end + # # this leaks the result set # def test_cleanup_unclosed_resultset_object # db = SQLite3::Database.new(':memory:') # db.execute('create table foo(text BLOB)') # stmt = db.prepare('select * from foo') # stmt.execute # end + + # # this leaks the incompletely-closed connection + # def test_cleanup_discarded_connections + # FileUtils.rm_f "test.db" + # db = SQLite3::Database.new("test.db") + # db.execute("create table posts (title text)") + # db.execute("insert into posts (title) values ('hello')") + # db.close + # 100.times do + # db = SQLite3::Database.new("test.db") + # db.execute("select * from posts limit 1") + # stmt = db.prepare("select * from posts") + # stmt.execute + # stmt.close + # db.discard + # end + # ensure + # FileUtils.rm_f "test.db" + # end end end diff --git a/test/test_statement.rb b/test/test_statement.rb index 7d582bd8..b6a55001 100644 --- a/test/test_statement.rb +++ b/test/test_statement.rb @@ -135,6 +135,13 @@ def test_new_closed_handle end end + def test_closed_db_behavior + @db.close + result = nil + assert_nothing_raised { result = @stmt.execute } + refute_nil result + end + def test_new_with_remainder stmt = SQLite3::Statement.new(@db, "select 'foo';bar") assert_equal "bar", stmt.remainder