From eefa4de52fe6e36f01aeb42580bb32240ca98bad Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 19 Aug 2024 05:56:06 +0000 Subject: [PATCH 01/27] build(deps-dev): update minitest requirement from 5.24.1 to 5.25.1 Updates the requirements on [minitest](https://github.com/minitest/minitest) to permit the latest version. - [Changelog](https://github.com/minitest/minitest/blob/master/History.rdoc) - [Commits](https://github.com/minitest/minitest/compare/v5.24.1...v5.25.1) --- updated-dependencies: - dependency-name: minitest dependency-type: direct:development ... Signed-off-by: dependabot[bot] --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From d9d5b32599044098b07ce5ea5df0fbf3f58ab39a Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 9 Sep 2024 12:57:04 -0700 Subject: [PATCH 02/27] Use write barrier when setting busy_handler The Database type struct is marked as WB_PROTECTED which means that any new references added to the object need to fire the write barrier. Previously we were missing this when setting the busy_handler. --- ext/sqlite3/database.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index d61bf0ac..9c730e30 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -246,7 +246,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, From f759e82ea847a15bdd19b58474419c7f6f431d97 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 9 Sep 2024 13:02:22 -0700 Subject: [PATCH 03/27] Remove @busy_handler = nil We no longer use this instance variable. This is just cleanup, I dont believe it caused any issues. --- lib/sqlite3/database.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/sqlite3/database.rb b/lib/sqlite3/database.rb index 39ace568..c4a490ab 100644 --- a/lib/sqlite3/database.rb +++ b/lib/sqlite3/database.rb @@ -127,7 +127,6 @@ def initialize file, options = {}, zvfs = nil @tracefunc = nil @authorizer = nil - @busy_handler = nil @progress_handler = nil @collations = {} @functions = {} From da908657099a20f2796e49e6ed42c102a9aa7a80 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 14 Sep 2024 22:26:11 -0400 Subject: [PATCH 04/27] Use sqlite3_close_v2 to close databases. 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. --- CHANGELOG.md | 7 +++++++ ext/sqlite3/database.c | 4 ++-- test/test_database.rb | 4 +--- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a7e650..7d30b22a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # sqlite3-ruby Changelog +## next / unreleased + +### 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. @flavorjones + + ## 2.0.4 / 2024-08-13 ### Dependencies diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 9c730e30..2ad07de5 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -25,7 +25,7 @@ deallocate(void *ctx) sqlite3RubyPtr c = (sqlite3RubyPtr)ctx; sqlite3 *db = c->db; - if (db) { sqlite3_close(db); } + if (db) { sqlite3_close_v2(db); } xfree(c); } @@ -131,7 +131,7 @@ sqlite3_rb_close(VALUE self) TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); db = ctx->db; - CHECK(db, sqlite3_close(ctx->db)); + CHECK(db, sqlite3_close_v2(ctx->db)); ctx->db = NULL; diff --git a/test/test_database.rb b/test/test_database.rb index 6662eab9..16a80906 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -611,9 +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 - @db.close - end + @db.close # refute_raises(SQLite3::BusyException) ensure s&.close end From 6c274b429ab6b4ba25e7bd48f9831b367a1cc9a9 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 15 Sep 2024 03:01:44 -0400 Subject: [PATCH 05/27] doc: update CHANGELOG --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d30b22a..e91dbdac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ ### 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. @flavorjones +- 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 ## 2.0.4 / 2024-08-13 From 6c84e935740d177ec93d623b5a49fd72e1e9e76b Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 16 Sep 2024 09:00:44 -0400 Subject: [PATCH 06/27] test: use assert_nothing_raised --- test/test_database.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test_database.rb b/test/test_database.rb index 16a80906..205bedeb 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -611,7 +611,9 @@ def call action, a, b, c, d def test_close_with_open_statements s = @db.prepare("select 'foo'") - @db.close # refute_raises(SQLite3::BusyException) + assert_nothing_raised do # formerly raised SQLite3::BusyException + @db.close + end ensure s&.close end From 3f4c50b7b2f9dcfb9060b6f38178b5650e5e47c5 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 16 Sep 2024 09:16:45 -0400 Subject: [PATCH 07/27] test: unskip float test for issue fixed in 3.43.1 --- test/test_database.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_database.rb b/test/test_database.rb index 205bedeb..19723478 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -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)") From 4968ca4d95b3f92bb1874bfe8d8848011e9eded6 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 13:08:46 -0400 Subject: [PATCH 08/27] dev: fixup .editorconfig [skip ci] --- .editorconfig | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) 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 From 2bb2341ca754f324affda9dad7b6a179dcfd59d3 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 16 Sep 2024 17:30:58 -0400 Subject: [PATCH 09/27] Database connections carried across fork() will not be fully closed Sqlite is not fork-safe and closing the database connection in the child process can lead to corruption. Emit a warning when this happens so developers know when they're doing something that's not supported by sqlite. See adr/2024-09-fork-safety.md for a full explanation. --- CHANGELOG.md | 5 +++ README.md | 17 +++++++++ adr/2024-09-fork-safety.md | 65 +++++++++++++++++++++++++++++++++++ ext/sqlite3/database.c | 61 ++++++++++++++++++++++++-------- ext/sqlite3/database.h | 1 + test/helper.rb | 9 ++--- test/test_database.rb | 38 ++++++++++++++++++++ test/test_resource_cleanup.rb | 20 +++++++++++ 8 files changed, 197 insertions(+), 19 deletions(-) create mode 100644 adr/2024-09-fork-safety.md diff --git a/CHANGELOG.md b/CHANGELOG.md index e91dbdac..a7047ed4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## next / unreleased +### Changed + +- Any database connections carried across a `fork()` will not be fully closed to help protect database files against corruption. Using a database connection in a child process that was created in a parent process is unsafe and may corrupt the database file. If an inherited connection is closed then a warning will be emitted and some reserved memory will be lost to the child process permanently. See the README "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558] @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 diff --git a/README.md b/README.md index be332c20..6dbce0ec 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 you should not carry an open database connection across a `fork()`. Using an inherited +connection in the child may corrupt your database, leak memory, or cause other undefined behavior. + +Instead, whenever possible, close connections in the parent before forking. + +If that's not possible or convenient, then immediately close any inherited connections in the child +after forking, before opening any new connections. This will incur a small one-time memory leak per +connection, but that's preferable to potentially corrupting your database. + +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..51effb0f --- /dev/null +++ b/adr/2024-09-fork-safety.md @@ -0,0 +1,65 @@ + +# 2024-09 Discard database connections when carried across fork()of fork safety + +## 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. + + +## Decision + +Open database connections carried across a `fork()` will not be fully closed in the child process, to avoid the risk of corrupting the database file. + +The sqlite3-ruby gem will track the ID of the process that opened each database connection. If, when the database is closed (either explicitly with `Database#close` or implicitly via GC) the current process ID is different from the original process, then we "discard" the connection. + +"Discard" here means: + +- The `Database` object acts "closed", including returning `true` from `#closed?`. +- `sqlite3_close_v2` is not called on the object, because it is unsafe to do so per sqlite instructions[^howto]. As a result, some memory will be lost permanently (a one-time "memory leak"). +- Open file descriptors associated with the database are closed. + + +## 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. + + +## 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) +- TODO rails pr implementing sqlite3adapter discard + + +## 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/ext/sqlite3/database.c b/ext/sqlite3/database.c index 2ad07de5..140d48b2 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -12,6 +12,39 @@ VALUE cSqlite3Database; +static void +close_or_discard_db(sqlite3RubyPtr ctx) +{ + if (ctx->db) { + if (ctx->owner == getpid()) { + // Ordinary close. + sqlite3_close_v2(ctx->db); + } else { + // This is an open connection carried across a fork(). + // "Discard" it. See adr/2024-09-fork-safety.md + sqlite3_file *sfile; + int status; + + rb_warning("An open sqlite database connection was inherited from a forked process and " + "is being discarded. This is a memory leak. If possible, please close all sqlite " + "database connections before forking."); + + // close the open file descriptors + status = sqlite3_file_control(ctx->db, NULL, SQLITE_FCNTL_FILE_POINTER, &sfile); + if (status == 0 && sfile->pMethods != NULL) { + sfile->pMethods->xClose(sfile); + } + + 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; + } +} + + static void database_mark(void *ctx) { @@ -22,11 +55,8 @@ database_mark(void *ctx) static void deallocate(void *ctx) { - sqlite3RubyPtr c = (sqlite3RubyPtr)ctx; - sqlite3 *db = c->db; - - if (db) { sqlite3_close_v2(db); } - xfree(c); + close_or_discard_db((sqlite3RubyPtr)ctx); + xfree(ctx); } static size_t @@ -51,7 +81,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 +94,6 @@ utf16_string_value_ptr(VALUE str) return RSTRING_PTR(str); } -static VALUE sqlite3_rb_close(VALUE self); - sqlite3RubyPtr sqlite3_database_unwrap(VALUE database) { @@ -119,21 +149,22 @@ rb_sqlite3_disable_quirk_mode(VALUE self) #endif } -/* call-seq: db.close +/* + * Close the database and release all associated resources. * - * Closes this database. + * ⚠ If the process that created the database forks a child process, and this method is called + * from the child process, then this method will _not_ free memory resources and instead will + * call discard. This is a memory leak, but is safer than risking database corruption. + * + * See 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_v2(ctx->db)); - - ctx->db = NULL; + close_or_discard_db(ctx); rb_iv_set(self, "-aggregators", Qnil); diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h index 3123f4fe..d3ede88f 100644 --- a/ext/sqlite3/database.h +++ b/ext/sqlite3/database.h @@ -8,6 +8,7 @@ struct _sqlite3Ruby { VALUE busy_handler; int stmt_timeout; struct timespec stmt_deadline; + rb_pid_t owner; }; typedef struct _sqlite3Ruby sqlite3Ruby; 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 19723478..2685cc21 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -721,5 +721,43 @@ def test_transaction_returns_block_result result = @db.transaction { :foo } assert_equal :foo, result end + + def test_discard_a_connection + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + begin + read, write = IO.pipe + + db = SQLite3::Database.new("test.db") + Process.fork do + $stderr = StringIO.new + + result = db.close + + write.write((result == db) ? "ok\n" : "fail\n") + write.write(db.closed? ? "ok\n" : "fail\n") + write.write($stderr.string) + + write.close + read.close + exit! + end + + assert_equal("ok", read.readline.chomp, "return value was not the database") + assert_equal("ok", read.readline.chomp, "closed? did not return true") + assert_match( + /warning: An open sqlite database connection was inherited from a forked process/, + read.readline, + "expected warning was not emitted" + ) + + write.close + read.close + ensure + db.close + FileUtils.rm_f("test.db") + 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 From 9c34e52f64cf5a25898e8e19bc24f749c38ebb70 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 12:52:18 -0400 Subject: [PATCH 10/27] Make sure we close all database file descriptors. Iterate through the names returned by sqlite3_db_name which will include names given via "ATTACH DATABASE ... AS" and any temp databases. Note that sqlite3_db_name was not supported before 3.39.0. --- ext/sqlite3/database.c | 13 ++++++++++++- ext/sqlite3/extconf.rb | 2 ++ test/test_database.rb | 18 +++++++++++------- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 140d48b2..ebd2ee5e 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -29,11 +29,22 @@ close_or_discard_db(sqlite3RubyPtr ctx) "is being discarded. This is a memory leak. If possible, please close all sqlite " "database connections before forking."); - // close the open 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) { 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/test/test_database.rb b/test/test_database.rb index 2685cc21..1712f8f7 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -730,7 +730,9 @@ def test_discard_a_connection read, write = IO.pipe db = SQLite3::Database.new("test.db") + db.execute("attach database 'test.db' as 'foo';") # exercise sqlite3_db_name() Process.fork do + read.close $stderr = StringIO.new result = db.close @@ -740,20 +742,22 @@ def test_discard_a_connection write.write($stderr.string) write.close - read.close exit! end + write.close + + assert1, assert2, *stderr = *read.readlines + read.close - assert_equal("ok", read.readline.chomp, "return value was not the database") - assert_equal("ok", read.readline.chomp, "closed? did not return true") + assert_equal("ok", assert1.chomp, "return value was not the database") + assert_equal("ok", assert2.chomp, "closed? did not return true") + + assert_equal(1, stderr.count, "unexpected output on stderr: #{stderr.inspect}") assert_match( /warning: An open sqlite database connection was inherited from a forked process/, - read.readline, + stderr.first, "expected warning was not emitted" ) - - write.close - read.close ensure db.close FileUtils.rm_f("test.db") From f1d4bce2b7e649aa71fd3946a15ca03036bb53b4 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 13:36:35 -0400 Subject: [PATCH 11/27] Call sqlite3_db_release_memory when we discard a connection. https://www.sqlite.org/capi3ref.html#sqlite3_db_release_memory --- ext/sqlite3/database.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index ebd2ee5e..60985971 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -29,6 +29,12 @@ close_or_discard_db(sqlite3RubyPtr ctx) "is being discarded. This is a memory leak. If possible, please close all sqlite " "database connections before forking."); + // 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. + sqlite3_db_release_memory(ctx->db); + + // release file descriptors #ifdef HAVE_SQLITE3_DB_NAME const char *db_name; int j_db = 0; From 5e368830b5083f2b3a647e8a4eed668043a1f541 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 15:59:35 -0400 Subject: [PATCH 12/27] Automatically close any open writable connections after a fork. --- CHANGELOG.md | 11 +++- README.md | 10 +-- adr/2024-09-fork-safety.md | 14 ++-- ext/sqlite3/database.c | 29 +++++--- ext/sqlite3/database.h | 1 + lib/sqlite3/database.rb | 3 + lib/sqlite3/fork_safety.rb | 43 ++++++++++++ sqlite3.gemspec | 1 + test/test_database.rb | 132 +++++++++++++++++++++++++++++++++---- 9 files changed, 211 insertions(+), 33 deletions(-) create mode 100644 lib/sqlite3/fork_safety.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index a7047ed4..9c584b9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,16 @@ ## next / unreleased -### Changed +### 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: + +- 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. -- Any database connections carried across a `fork()` will not be fully closed to help protect database files against corruption. Using a database connection in a child process that was created in a parent process is unsafe and may corrupt the database file. If an inherited connection is closed then a warning will be emitted and some reserved memory will be lost to the child process permanently. See the README "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558] @flavorjones +See the README "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558] @flavorjones ### Improved diff --git a/README.md b/README.md index 6dbce0ec..4e60f238 100644 --- a/README.md +++ b/README.md @@ -153,14 +153,14 @@ fine to share, but may require manual locking for thread safety. [Sqlite is not fork safe](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_) -and you should not carry an open database connection across a `fork()`. Using an inherited +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. -Instead, whenever possible, close connections in the parent before forking. +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. -If that's not possible or convenient, then immediately close any inherited connections in the child -after forking, before opening any new connections. This 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. 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. See [./adr/2024-09-fork-safety.md](./adr/2024-09-fork-safety.md) for more information and context. diff --git a/adr/2024-09-fork-safety.md b/adr/2024-09-fork-safety.md index 51effb0f..d369753d 100644 --- a/adr/2024-09-fork-safety.md +++ b/adr/2024-09-fork-safety.md @@ -1,5 +1,5 @@ -# 2024-09 Discard database connections when carried across fork()of fork safety +# 2024-09 Automatically close database connections when carried across fork() ## Status @@ -15,17 +15,23 @@ SQLite is known to not be fork-safe[^howto], so this was not entirely surprising 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. -## Decision +## Decisions -Open database connections carried across a `fork()` will not be fully closed in the child process, to avoid the risk of corrupting the database file. +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. -The sqlite3-ruby gem will track the ID of the process that opened each database connection. If, when the database is closed (either explicitly with `Database#close` or implicitly via GC) the current process ID is different from the original process, then we "discard" the connection. +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: - The `Database` object acts "closed", including returning `true` from `#closed?`. - `sqlite3_close_v2` is not called on the object, because it is unsafe to do so per sqlite instructions[^howto]. As a result, some memory will be lost permanently (a one-time "memory leak"). - Open file descriptors associated with the database are closed. +- Any memory that can be freed safely is recovered. + +Note that readonly databases are being treated as "fork safe" and are not affected by any of these changes. ## Consequences diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 60985971..2c9b79ea 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -16,7 +16,9 @@ static void close_or_discard_db(sqlite3RubyPtr ctx) { if (ctx->db) { - if (ctx->owner == getpid()) { + int isReadonly = (ctx->flags & SQLITE_OPEN_READONLY); + + if (isReadonly || ctx->owner == getpid()) { // Ordinary close. sqlite3_close_v2(ctx->db); } else { @@ -25,9 +27,10 @@ close_or_discard_db(sqlite3RubyPtr ctx) sqlite3_file *sfile; int status; - rb_warning("An open sqlite database connection was inherited from a forked process and " - "is being discarded. This is a memory leak. If possible, please close all sqlite " - "database connections before forking."); + rb_warning("An open writable sqlite database connection was inherited from a " + "forked process and is being closed to prevent the risk of corruption. " + "If possible, please close all writable sqlite database connections " + "before forking."); // 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 @@ -124,6 +127,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); @@ -136,14 +140,16 @@ 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); + ctx->flags = flags; return self; } @@ -169,9 +175,10 @@ rb_sqlite3_disable_quirk_mode(VALUE self) /* * Close the database and release all associated resources. * - * ⚠ If the process that created the database forks a child process, and this method is called - * from the child process, then this method will _not_ free memory resources and instead will - * call discard. This is a memory leak, but is safer than risking database corruption. + * ⚠ Writable connections that are carried across a +fork()+ are not completely closed. Sqlite does + * not support forking, 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 adr/2024-09-fork-safety.md for more information on fork safety. */ @@ -919,6 +926,10 @@ rb_sqlite3_open16(VALUE self, VALUE file) status = sqlite3_open16(utf16_string_value_ptr(file), &ctx->db); + // these are the perm flags used implicitly by sqlite3_open16, + // see https://www.sqlite.org/capi3ref.html#sqlite3_open + ctx->flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE; + CHECK(ctx->db, status) return INT2NUM(status); diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h index d3ede88f..1ef7b245 100644 --- a/ext/sqlite3/database.h +++ b/ext/sqlite3/database.h @@ -9,6 +9,7 @@ struct _sqlite3Ruby { int stmt_timeout; struct timespec stmt_deadline; rb_pid_t owner; + int flags; }; typedef struct _sqlite3Ruby sqlite3Ruby; diff --git a/lib/sqlite3/database.rb b/lib/sqlite3/database.rb index c4a490ab..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. @@ -134,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..bdb11797 --- /dev/null +++ b/lib/sqlite3/fork_safety.rb @@ -0,0 +1,43 @@ +# 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 = [] + + class << self + def hook! + ::Process.singleton_class.prepend(CoreExt) + end + + def track(database) + @databases << WeakRef.new(database) + end + + def discard + @databases.each do |db| + next unless db.weakref_alive? + + unless db.closed? || db.readonly? + db.close + end + end + @databases.clear + end + end + end +end + +SQLite3::ForkSafety.hook! diff --git a/sqlite3.gemspec b/sqlite3.gemspec index 57f5d61b..4174ba12 100644 --- a/sqlite3.gemspec +++ b/sqlite3.gemspec @@ -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/test_database.rb b/test/test_database.rb index 1712f8f7..0dbb0b37 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -721,40 +721,38 @@ def test_transaction_returns_block_result result = @db.transaction { :foo } assert_equal :foo, result end + end - def test_discard_a_connection + class TestDiscardDatabase < SQLite3::TestCase + 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 + skip("ruby 3.0 doesn't have Process._fork") if RUBY_VERSION < "3.1.0" + GC.start begin + db = SQLite3::Database.new("test.db") read, write = IO.pipe - db = SQLite3::Database.new("test.db") - db.execute("attach database 'test.db' as 'foo';") # exercise sqlite3_db_name() + old_stderr, $stderr = $stderr, StringIO.new Process.fork do read.close - $stderr = StringIO.new - result = db.close - - write.write((result == db) ? "ok\n" : "fail\n") write.write(db.closed? ? "ok\n" : "fail\n") write.write($stderr.string) write.close exit! end + $stderr = old_stderr write.close - - assert1, assert2, *stderr = *read.readlines + assertion, *stderr = *read.readlines read.close - assert_equal("ok", assert1.chomp, "return value was not the database") - assert_equal("ok", assert2.chomp, "closed? did not return true") - + assert_equal("ok", assertion.chomp, "closed? did not return true") assert_equal(1, stderr.count, "unexpected output on stderr: #{stderr.inspect}") assert_match( - /warning: An open sqlite database connection was inherited from a forked process/, + /warning: An open writable sqlite database connection was inherited from a forked process/, stderr.first, "expected warning was not emitted" ) @@ -763,5 +761,113 @@ def test_discard_a_connection FileUtils.rm_f("test.db") 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("test.db") + read, write = IO.pipe + + db.close + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + write.write($stderr.string) + + write.close + exit! + end + $stderr = old_stderr + write.close + stderr = read.readlines + read.close + + assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + ensure + db.close + FileUtils.rm_f("test.db") + 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("test.db") do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new("test.db", readonly: true) + read, write = IO.pipe + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + 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) + + write.close + exit! + end + $stderr = old_stderr + write.close + assertion1, assertion2, *stderr = *read.readlines + read.close + + assert_equal("ok", assertion1.chomp, "closed? did not return false") + assert_equal("ok", assertion2.chomp, "could not read from database") + assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + ensure + db&.close + FileUtils.rm_f("test.db") + 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("test.db") do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new("test.db", readonly: true) + read, write = IO.pipe + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + db.close + + write.write($stderr.string) + + write.close + exit! + end + $stderr = old_stderr + write.close + stderr = read.readlines + read.close + + assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + ensure + db&.close + FileUtils.rm_f("test.db") + end + end end end From 84cf3a8cc25d72b54675737c844f1a62f46cbcde Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 17:03:36 -0400 Subject: [PATCH 13/27] SQLite::ForkSafety collection is thread-safe --- lib/sqlite3/fork_safety.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/sqlite3/fork_safety.rb b/lib/sqlite3/fork_safety.rb index bdb11797..7fb526d1 100644 --- a/lib/sqlite3/fork_safety.rb +++ b/lib/sqlite3/fork_safety.rb @@ -16,6 +16,7 @@ def _fork end @databases = [] + @mutex = Mutex.new class << self def hook! @@ -23,7 +24,9 @@ def hook! end def track(database) - @databases << WeakRef.new(database) + @mutex.synchronize do + @databases << WeakRef.new(database) + end end def discard From f34e812e50d4b686bcaf32beb2c35233f25873a8 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 17:24:43 -0400 Subject: [PATCH 14/27] Move the discard warning into ForkSafety - Make sure it's only emitted once per fork. - Try to clarify the warning message. --- ext/sqlite3/database.c | 5 ----- lib/sqlite3/fork_safety.rb | 10 ++++++++++ test/test_database.rb | 2 +- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 2c9b79ea..949c721e 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -27,11 +27,6 @@ close_or_discard_db(sqlite3RubyPtr ctx) sqlite3_file *sfile; int status; - rb_warning("An open writable sqlite database connection was inherited from a " - "forked process and is being closed to prevent the risk of corruption. " - "If possible, please close all writable sqlite database connections " - "before forking."); - // 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. diff --git a/lib/sqlite3/fork_safety.rb b/lib/sqlite3/fork_safety.rb index 7fb526d1..4f40f4f6 100644 --- a/lib/sqlite3/fork_safety.rb +++ b/lib/sqlite3/fork_safety.rb @@ -30,10 +30,20 @@ def track(database) end def discard + warned = false @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("#{__FILE__}:#{__LINE__}: warning: " \ + "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.") + warned = true + end db.close end end diff --git a/test/test_database.rb b/test/test_database.rb index 0dbb0b37..5de49590 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -752,7 +752,7 @@ def test_fork_discards_an_open_readwrite_connection assert_equal("ok", assertion.chomp, "closed? did not return true") assert_equal(1, stderr.count, "unexpected output on stderr: #{stderr.inspect}") assert_match( - /warning: An open writable sqlite database connection was inherited from a forked process/, + /warning: Writable sqlite database connection\(s\) were inherited from a forked process/, stderr.first, "expected warning was not emitted" ) From 20025b40accbf37ec4fa4d1ac594eb5bc3787f03 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 17 Sep 2024 18:11:02 -0400 Subject: [PATCH 15/27] Statements attached to a discarded db raise an exception --- ext/sqlite3/database.c | 90 ++++++++++++++++++++++++++--------------- ext/sqlite3/statement.c | 42 +++++++++++++++++++ test/test_database.rb | 22 ++++++++++ 3 files changed, 121 insertions(+), 33 deletions(-) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 949c721e..e0c826e7 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -12,6 +12,45 @@ 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; +} + static void close_or_discard_db(sqlite3RubyPtr ctx) { @@ -21,41 +60,11 @@ close_or_discard_db(sqlite3RubyPtr ctx) if (isReadonly || 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. See adr/2024-09-fork-safety.md - 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. - 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); - } + // This is an open connection carried across a fork(). "Discard" it. + discard_db(ctx); } - ctx->db = NULL; } } @@ -190,6 +199,20 @@ sqlite3_rb_close(VALUE self) 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); + + return self; +} + /* call-seq: db.closed? * * Returns +true+ if this database instance has been closed (see #close). @@ -943,6 +966,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/statement.c b/ext/sqlite3/statement.c index cb65efb7..690cd0f8 100644 --- a/ext/sqlite3/statement.c +++ b/ext/sqlite3/statement.c @@ -4,6 +4,20 @@ if(!_ctxt->st) \ rb_raise(rb_path2class("SQLite3::Exception"), "cannot use a closed statement"); +static void +require_open_db(VALUE stmt_rb) +{ + VALUE closed_p = rb_funcall( + rb_iv_get(stmt_rb, "@connection"), + rb_intern("closed?"), 0); + + if (RTEST(closed_p)) { + rb_raise(rb_path2class("SQLite3::Exception"), + "cannot use a statement associated with a closed database"); + } +} + + VALUE cSqlite3Statement; static void @@ -121,6 +135,7 @@ step(VALUE self) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + require_open_db(self); REQUIRE_OPEN_STMT(ctx); if (ctx->done_p) { return Qnil; } @@ -216,6 +231,8 @@ bind_param(VALUE self, VALUE key, VALUE value) int index; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); switch (TYPE(key)) { @@ -308,6 +325,8 @@ reset_bang(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); sqlite3_reset(ctx->st); @@ -328,6 +347,8 @@ clear_bindings_bang(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); sqlite3_clear_bindings(ctx->st); @@ -360,6 +381,8 @@ column_count(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_column_count(ctx->st)); @@ -391,6 +414,8 @@ column_name(VALUE self, VALUE index) const char *name; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); name = sqlite3_column_name(ctx->st, (int)NUM2INT(index)); @@ -414,6 +439,8 @@ column_decltype(VALUE self, VALUE index) const char *name; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); name = sqlite3_column_decltype(ctx->st, (int)NUM2INT(index)); @@ -431,6 +458,8 @@ bind_parameter_count(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_bind_parameter_count(ctx->st)); @@ -538,7 +567,10 @@ stats_as_hash(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); + VALUE arg = rb_hash_new(); stmt_stat_internal(arg, ctx->st); @@ -554,6 +586,8 @@ stat_for(VALUE self, VALUE key) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); if (SYMBOL_P(key)) { @@ -574,6 +608,8 @@ memused(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_stmt_status(ctx->st, SQLITE_STMTSTATUS_MEMUSED, 0)); @@ -591,6 +627,8 @@ database_name(VALUE self, VALUE index) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); return SQLITE3_UTF8_STR_NEW2( @@ -608,6 +646,8 @@ get_sql(VALUE self) { sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); return rb_obj_freeze(SQLITE3_UTF8_STR_NEW2(sqlite3_sql(ctx->st))); @@ -626,6 +666,8 @@ get_expanded_sql(VALUE self) VALUE rb_expanded_sql; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); + + require_open_db(self); REQUIRE_OPEN_STMT(ctx); expanded_sql = sqlite3_expanded_sql(ctx->st); diff --git a/test/test_database.rb b/test/test_database.rb index 5de49590..8e6aaeb2 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -869,5 +869,27 @@ def test_close_does_not_discard_readonly_connections FileUtils.rm_f("test.db") end end + + def test_a_discarded_connection_with_statements + skip("discard leaks memory") if i_am_running_in_valgrind + + begin + db = SQLite3::Database.new("test.db") + 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 closed database/, e.message) + + assert_nothing_raised { stmt.close } + assert_predicate(stmt, :closed?) + ensure + db.close + FileUtils.rm_f("test.db") + end + end end end From 2933f27be5f7391f806f4802d673dcfabca03a49 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Sep 2024 08:30:23 -0400 Subject: [PATCH 16/27] Clean up docs, move discard tests to their own file --- README.md | 6 +- adr/2024-09-fork-safety.md | 13 +-- ext/sqlite3/database.c | 11 +-- lib/sqlite3/fork_safety.rb | 6 +- test/test_database.rb | 170 ------------------------------------ test/test_discarding.rb | 173 +++++++++++++++++++++++++++++++++++++ 6 files changed, 192 insertions(+), 187 deletions(-) create mode 100644 test/test_discarding.rb diff --git a/README.md b/README.md index 4e60f238..feeb2d2a 100644 --- a/README.md +++ b/README.md @@ -156,12 +156,12 @@ safe](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connec 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. - -Whenever possible, close writable connections in the parent before forking. Discarding writable +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. + See [./adr/2024-09-fork-safety.md](./adr/2024-09-fork-safety.md) for more information and context. diff --git a/adr/2024-09-fork-safety.md b/adr/2024-09-fork-safety.md index d369753d..b5b26c37 100644 --- a/adr/2024-09-fork-safety.md +++ b/adr/2024-09-fork-safety.md @@ -26,19 +26,21 @@ Second, the sqlite3-ruby gem will store the ID of the process that opened each d "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?`. -- `sqlite3_close_v2` is not called on the object, because it is unsafe to do so per sqlite instructions[^howto]. As a result, some memory will be lost permanently (a one-time "memory leak"). -- Open file descriptors associated with the database are closed. -- Any memory that can be freed safely is recovered. +- 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 any of these changes. +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. +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. @@ -57,7 +59,6 @@ I think this approach is promising, but complex and risky. Sqlite is a complex l ## 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) -- TODO rails pr implementing sqlite3adapter discard ## Footnotes diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index e0c826e7..8e833faf 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -179,12 +179,13 @@ rb_sqlite3_disable_quirk_mode(VALUE self) /* * Close the database and release all associated resources. * - * ⚠ Writable connections that are carried across a +fork()+ are not completely closed. Sqlite does - * not support forking, 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. + * ⚠ 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 adr/2024-09-fork-safety.md for more information on fork safety. + * See rdoc-ref:adr/2024-09-fork-safety.md for more information on fork safety. */ static VALUE sqlite3_rb_close(VALUE self) diff --git a/lib/sqlite3/fork_safety.rb b/lib/sqlite3/fork_safety.rb index 4f40f4f6..a39ce159 100644 --- a/lib/sqlite3/fork_safety.rb +++ b/lib/sqlite3/fork_safety.rb @@ -38,10 +38,10 @@ def discard unless warned # If you are here, you may want to read # https://github.com/sparklemotion/sqlite3-ruby/pull/558 - warn("#{__FILE__}:#{__LINE__}: warning: " \ - "Writable sqlite database connection(s) were inherited from a forked process. " \ + 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.") + "corruption. Please close writable sqlite database connections before forking.", + uplevel: 0) warned = true end db.close diff --git a/test/test_database.rb b/test/test_database.rb index 8e6aaeb2..19723478 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -722,174 +722,4 @@ def test_transaction_returns_block_result assert_equal :foo, result end end - - class TestDiscardDatabase < SQLite3::TestCase - 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 - skip("ruby 3.0 doesn't have Process._fork") if RUBY_VERSION < "3.1.0" - - GC.start - begin - db = SQLite3::Database.new("test.db") - read, write = IO.pipe - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close - - write.write(db.closed? ? "ok\n" : "fail\n") - write.write($stderr.string) - - write.close - exit! - end - $stderr = old_stderr - write.close - assertion, *stderr = *read.readlines - read.close - - 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 - FileUtils.rm_f("test.db") - 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("test.db") - read, write = IO.pipe - - db.close - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close - - write.write($stderr.string) - - write.close - exit! - end - $stderr = old_stderr - write.close - stderr = read.readlines - read.close - - assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") - ensure - db.close - FileUtils.rm_f("test.db") - 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("test.db") do |db| - db.execute("create table foo (bar int)") - db.execute("insert into foo values (1)") - end - - db = SQLite3::Database.new("test.db", readonly: true) - read, write = IO.pipe - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close - - 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) - - write.close - exit! - end - $stderr = old_stderr - write.close - assertion1, assertion2, *stderr = *read.readlines - read.close - - assert_equal("ok", assertion1.chomp, "closed? did not return false") - assert_equal("ok", assertion2.chomp, "could not read from database") - assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") - ensure - db&.close - FileUtils.rm_f("test.db") - 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("test.db") do |db| - db.execute("create table foo (bar int)") - db.execute("insert into foo values (1)") - end - - db = SQLite3::Database.new("test.db", readonly: true) - read, write = IO.pipe - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close - - db.close - - write.write($stderr.string) - - write.close - exit! - end - $stderr = old_stderr - write.close - stderr = read.readlines - read.close - - assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") - ensure - db&.close - FileUtils.rm_f("test.db") - end - end - - def test_a_discarded_connection_with_statements - skip("discard leaks memory") if i_am_running_in_valgrind - - begin - db = SQLite3::Database.new("test.db") - 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 closed database/, e.message) - - assert_nothing_raised { stmt.close } - assert_predicate(stmt, :closed?) - ensure - db.close - FileUtils.rm_f("test.db") - end - end - end end diff --git a/test/test_discarding.rb b/test/test_discarding.rb new file mode 100644 index 00000000..9cc27f3e --- /dev/null +++ b/test/test_discarding.rb @@ -0,0 +1,173 @@ +require_relative "helper" + +module SQLite3 + class TestDiscardDatabase < SQLite3::TestCase + 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 + skip("ruby 3.0 doesn't have Process._fork") if RUBY_VERSION < "3.1.0" + + GC.start + begin + db = SQLite3::Database.new("test.db") + read, write = IO.pipe + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + write.write(db.closed? ? "ok\n" : "fail\n") + write.write($stderr.string) + + write.close + exit! + end + $stderr = old_stderr + write.close + assertion, *stderr = *read.readlines + read.close + + 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 + FileUtils.rm_f("test.db") + 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("test.db") + read, write = IO.pipe + + db.close + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + write.write($stderr.string) + + write.close + exit! + end + $stderr = old_stderr + write.close + stderr = read.readlines + read.close + + assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + ensure + db.close + FileUtils.rm_f("test.db") + 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("test.db") do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new("test.db", readonly: true) + read, write = IO.pipe + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + 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) + + write.close + exit! + end + $stderr = old_stderr + write.close + assertion1, assertion2, *stderr = *read.readlines + read.close + + assert_equal("ok", assertion1.chomp, "closed? did not return false") + assert_equal("ok", assertion2.chomp, "could not read from database") + assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + ensure + db&.close + FileUtils.rm_f("test.db") + 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("test.db") do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new("test.db", readonly: true) + read, write = IO.pipe + + old_stderr, $stderr = $stderr, StringIO.new + Process.fork do + read.close + + db.close + + write.write($stderr.string) + + write.close + exit! + end + $stderr = old_stderr + write.close + stderr = read.readlines + read.close + + assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + ensure + db&.close + FileUtils.rm_f("test.db") + end + end + + def test_a_discarded_connection_with_statements + skip("discard leaks memory") if i_am_running_in_valgrind + + begin + db = SQLite3::Database.new("test.db") + 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 closed database/, e.message) + + assert_nothing_raised { stmt.close } + assert_predicate(stmt, :closed?) + ensure + db.close + FileUtils.rm_f("test.db") + end + end + end +end From 7e97204ca8d99c8ca88d424b6390cdb5922c1c81 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Sep 2024 08:33:02 -0400 Subject: [PATCH 17/27] Simplify discard tests --- test/test_discarding.rb | 144 ++++++++++++++++++++-------------------- 1 file changed, 72 insertions(+), 72 deletions(-) diff --git a/test/test_discarding.rb b/test/test_discarding.rb index 9cc27f3e..20339ac1 100644 --- a/test/test_discarding.rb +++ b/test/test_discarding.rb @@ -2,6 +2,40 @@ 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 @@ -9,23 +43,13 @@ def test_fork_discards_an_open_readwrite_connection GC.start begin - db = SQLite3::Database.new("test.db") - read, write = IO.pipe - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close + db = SQLite3::Database.new(DBPATH) + in_a_forked_process do |write| write.write(db.closed? ? "ok\n" : "fail\n") - write.write($stderr.string) - - write.close - exit! end - $stderr = old_stderr - write.close - assertion, *stderr = *read.readlines - read.close + + assertion, *stderr = *@results assert_equal("ok", assertion.chomp, "closed? did not return true") assert_equal(1, stderr.count, "unexpected output on stderr: #{stderr.inspect}") @@ -35,8 +59,7 @@ def test_fork_discards_an_open_readwrite_connection "expected warning was not emitted" ) ensure - db.close - FileUtils.rm_f("test.db") + db&.close end end @@ -46,29 +69,22 @@ def test_fork_does_not_discard_closed_connections GC.start begin - db = SQLite3::Database.new("test.db") - read, write = IO.pipe - + db = SQLite3::Database.new(DBPATH) db.close - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close - - write.write($stderr.string) - - write.close - exit! + 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 - $stderr = old_stderr - write.close - stderr = read.readlines - read.close - assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + 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 - FileUtils.rm_f("test.db") + db&.close end end @@ -78,36 +94,28 @@ def test_fork_does_not_discard_readonly_connections GC.start begin - SQLite3::Database.open("test.db") do |db| + 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("test.db", readonly: true) - read, write = IO.pipe - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close + 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) - - write.close - exit! + write.write($stderr.string) # should be an empty write, no warnings emitted + write.write("done\n") end - $stderr = old_stderr - write.close - assertion1, assertion2, *stderr = *read.readlines - read.close + + 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(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + 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 - FileUtils.rm_f("test.db") end end @@ -117,34 +125,27 @@ def test_close_does_not_discard_readonly_connections GC.start begin - SQLite3::Database.open("test.db") do |db| + 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("test.db", readonly: true) - read, write = IO.pipe - - old_stderr, $stderr = $stderr, StringIO.new - Process.fork do - read.close + 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) - - write.close - exit! + write.write($stderr.string) # should be an empty write, no warnings emitted + write.write("done\n") end - $stderr = old_stderr - write.close - stderr = read.readlines - read.close - assert_equal(0, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + 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 - FileUtils.rm_f("test.db") end end @@ -152,7 +153,7 @@ def test_a_discarded_connection_with_statements skip("discard leaks memory") if i_am_running_in_valgrind begin - db = SQLite3::Database.new("test.db") + 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") @@ -165,8 +166,7 @@ def test_a_discarded_connection_with_statements assert_nothing_raised { stmt.close } assert_predicate(stmt, :closed?) ensure - db.close - FileUtils.rm_f("test.db") + db&.close end end end From 7655931394cfa2f3754f030ad937394d9c8c8d38 Mon Sep 17 00:00:00 2001 From: Rick Hull Date: Wed, 18 Sep 2024 09:13:12 -0400 Subject: [PATCH 18/27] Update FAQ.md (#562) * Update FAQ.md Updated several inaccuracies, per Issue #561 * fix typo --- FAQ.md | 77 ++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 43 insertions(+), 34 deletions(-) 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! From 4faeac7454bcbbfe310ed45f78a9d425661801bc Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Sep 2024 11:38:18 -0400 Subject: [PATCH 19/27] Drop support for Ruby 3.0 (#563) * Drop support for Ruby 3.0 Primarily because it doesn't support Process._fork, but also because it's been EOL for many months. * ci: bump fedora to 40 because fedora 35 distributes ruby 3.0 * Drop mingw32 (non-ucrt) native gems Because windows rubyinstaller builds with UCRT starting with ruby 3.1. --- .github/workflows/ci.yml | 21 +++++++-------------- .rubocop.yml | 2 +- CHANGELOG.md | 5 +++++ INSTALLATION.md | 4 +--- bin/test-gem-file-contents | 32 +++++--------------------------- rakelib/native.rake | 3 +-- sqlite3.gemspec | 2 +- test/test_discarding.rb | 1 - 8 files changed, 21 insertions(+), 49 deletions(-) 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 9c584b9e..5fc24cd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## next / unreleased +### 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: 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/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/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 4174ba12..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 = { diff --git a/test/test_discarding.rb b/test/test_discarding.rb index 20339ac1..d4fd05c9 100644 --- a/test/test_discarding.rb +++ b/test/test_discarding.rb @@ -39,7 +39,6 @@ def in_a_forked_process 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 - skip("ruby 3.0 doesn't have Process._fork") if RUBY_VERSION < "3.1.0" GC.start begin From 5f4b0aa5fcf33542bfa5e53ffc495aa993958c5b Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Sep 2024 12:08:54 -0400 Subject: [PATCH 20/27] doc: add a note to CONTRIBUTING.md about the /adr dir [skip ci] --- CONTRIBUTING.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 97f06197..969737a8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,6 +7,11 @@ 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. From 81ea485f680c54f604ec1d0f40a00d999a149dc0 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Sep 2024 12:10:26 -0400 Subject: [PATCH 21/27] version bump to v2.1.0.rc1 --- CHANGELOG.md | 4 ++-- lib/sqlite3/version.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fc24cd8..ee906359 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # sqlite3-ruby Changelog -## next / unreleased +## prerelease 2.1.0.rc1 / 2024-09-18 ### Ruby @@ -11,7 +11,7 @@ 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: -- 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. +- 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. diff --git a/lib/sqlite3/version.rb b/lib/sqlite3/version.rb index 6ad1f0f0..2ac1ff4b 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.rc1" end From e621d880a5b4441cd2837ff7b63fd1fd5a8411c6 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 19 Sep 2024 08:39:26 -0400 Subject: [PATCH 22/27] doc: update garbage collection description which hasn't been updated since 2010 [skip ci] --- CONTRIBUTING.md | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 969737a8..cccea932 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,20 +16,15 @@ please look there for additional information. 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: +connection alive. -1. `#close` is called on the statement -2. The `SQLite3::Database` object gets garbage collected - -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 From af548cf3fd9e186d64b6ae73f7701186f7cffca4 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Wed, 18 Sep 2024 22:23:50 -0400 Subject: [PATCH 23/27] Optimize the statement check for a non-discarded database This also restores the ability (now tested!) to call Database#close successfully and defer its cleanup until after Statements are closed, which was the promise of #557 and sqlite3_close_v2. Closes #564 --- ext/sqlite3/database.c | 16 +++++++------ ext/sqlite3/database.h | 4 ++++ ext/sqlite3/statement.c | 51 +++++++++++++++++++---------------------- ext/sqlite3/statement.h | 1 + test/test_discarding.rb | 2 +- test/test_statement.rb | 7 ++++++ 6 files changed, 45 insertions(+), 36 deletions(-) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 8e833faf..621dc7aa 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -49,15 +49,16 @@ discard_db(sqlite3RubyPtr ctx) } ctx->db = NULL; + ctx->flags |= SQLITE3_RB_DATABASE_DISCARDED; } static void close_or_discard_db(sqlite3RubyPtr ctx) { if (ctx->db) { - int isReadonly = (ctx->flags & SQLITE_OPEN_READONLY); + int is_readonly = (ctx->flags & SQLITE3_RB_DATABASE_READONLY); - if (isReadonly || ctx->owner == getpid()) { + if (is_readonly || ctx->owner == getpid()) { // Ordinary close. sqlite3_close_v2(ctx->db); ctx->db = NULL; @@ -153,7 +154,9 @@ rb_sqlite3_open_v2(VALUE self, VALUE file, VALUE mode, VALUE zvfs) ); CHECK(ctx->db, status); - ctx->flags = flags; + if (flags & SQLITE_OPEN_READONLY) { + ctx->flags |= SQLITE3_RB_DATABASE_READONLY; + } return self; } @@ -943,11 +946,10 @@ rb_sqlite3_open16(VALUE self, VALUE file) #endif #endif - status = sqlite3_open16(utf16_string_value_ptr(file), &ctx->db); - - // these are the perm flags used implicitly by sqlite3_open16, + // sqlite3_open16 implicitly uses flags (SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE) // see https://www.sqlite.org/capi3ref.html#sqlite3_open - ctx->flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE; + // 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) diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h index 1ef7b245..04124881 100644 --- a/ext/sqlite3/database.h +++ b/ext/sqlite3/database.h @@ -3,6 +3,10 @@ #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; diff --git a/ext/sqlite3/statement.c b/ext/sqlite3/statement.c index 690cd0f8..705b7679 100644 --- a/ext/sqlite3/statement.c +++ b/ext/sqlite3/statement.c @@ -1,22 +1,12 @@ #include #define REQUIRE_OPEN_STMT(_ctxt) \ - if(!_ctxt->st) \ + if (!_ctxt->st) \ rb_raise(rb_path2class("SQLite3::Exception"), "cannot use a closed statement"); -static void -require_open_db(VALUE stmt_rb) -{ - VALUE closed_p = rb_funcall( - rb_iv_get(stmt_rb, "@connection"), - rb_intern("closed?"), 0); - - if (RTEST(closed_p)) { - rb_raise(rb_path2class("SQLite3::Exception"), - "cannot use a statement associated with a closed database"); - } -} - +#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; @@ -71,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 @@ -135,7 +130,7 @@ step(VALUE self) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); if (ctx->done_p) { return Qnil; } @@ -232,7 +227,7 @@ bind_param(VALUE self, VALUE key, VALUE value) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); switch (TYPE(key)) { @@ -326,7 +321,7 @@ reset_bang(VALUE self) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); sqlite3_reset(ctx->st); @@ -348,7 +343,7 @@ clear_bindings_bang(VALUE self) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); sqlite3_clear_bindings(ctx->st); @@ -382,7 +377,7 @@ column_count(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_column_count(ctx->st)); @@ -415,7 +410,7 @@ column_name(VALUE self, VALUE index) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); name = sqlite3_column_name(ctx->st, (int)NUM2INT(index)); @@ -440,7 +435,7 @@ column_decltype(VALUE self, VALUE index) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); name = sqlite3_column_decltype(ctx->st, (int)NUM2INT(index)); @@ -459,7 +454,7 @@ bind_parameter_count(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_bind_parameter_count(ctx->st)); @@ -568,7 +563,7 @@ stats_as_hash(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); VALUE arg = rb_hash_new(); @@ -587,7 +582,7 @@ stat_for(VALUE self, VALUE key) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); if (SYMBOL_P(key)) { @@ -609,7 +604,7 @@ memused(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return INT2NUM(sqlite3_stmt_status(ctx->st, SQLITE_STMTSTATUS_MEMUSED, 0)); @@ -628,7 +623,7 @@ database_name(VALUE self, VALUE index) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return SQLITE3_UTF8_STR_NEW2( @@ -647,7 +642,7 @@ get_sql(VALUE self) sqlite3StmtRubyPtr ctx; TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + REQUIRE_LIVE_DB(ctx); REQUIRE_OPEN_STMT(ctx); return rb_obj_freeze(SQLITE3_UTF8_STR_NEW2(sqlite3_sql(ctx->st))); @@ -667,7 +662,7 @@ get_expanded_sql(VALUE self) TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx); - require_open_db(self); + 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/test/test_discarding.rb b/test/test_discarding.rb index d4fd05c9..5877c9a4 100644 --- a/test/test_discarding.rb +++ b/test/test_discarding.rb @@ -160,7 +160,7 @@ def test_a_discarded_connection_with_statements db.send(:discard) e = assert_raises(SQLite3::Exception) { stmt.execute } - assert_match(/cannot use a statement associated with a closed database/, e.message) + assert_match(/cannot use a statement associated with a discarded database/, e.message) assert_nothing_raised { stmt.close } assert_predicate(stmt, :closed?) 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 From 4b6d614aaecfea57dd98364b9c26a31996a2f500 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 19 Sep 2024 16:52:10 -0400 Subject: [PATCH 24/27] version bump to v2.1.0.rc2 --- CHANGELOG.md | 7 +++++++ lib/sqlite3/version.rb | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee906359..bdac1d58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # sqlite3-ruby Changelog +## prerelease 2.1.0.rc2 / 2024-09-18 + +### Improved + +- Address a performance regression in 2.1.0.rc1. + + ## prerelease 2.1.0.rc1 / 2024-09-18 ### Ruby diff --git a/lib/sqlite3/version.rb b/lib/sqlite3/version.rb index 2ac1ff4b..099d14a2 100644 --- a/lib/sqlite3/version.rb +++ b/lib/sqlite3/version.rb @@ -1,3 +1,3 @@ module SQLite3 - VERSION = "2.1.0.rc1" + VERSION = "2.1.0.rc2" end From c90b1772849c53fc118ac38c93dedecd81d46a29 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 24 Sep 2024 16:08:41 -0400 Subject: [PATCH 25/27] feat: SQLite3::ForkSafety.suppress_warnings! For frameworks like Rails where it's expected to sometimes fork with open writable connections. --- lib/sqlite3/fork_safety.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/sqlite3/fork_safety.rb b/lib/sqlite3/fork_safety.rb index a39ce159..69cf6ac3 100644 --- a/lib/sqlite3/fork_safety.rb +++ b/lib/sqlite3/fork_safety.rb @@ -17,6 +17,7 @@ def _fork @databases = [] @mutex = Mutex.new + @suppress = false class << self def hook! @@ -30,7 +31,7 @@ def track(database) end def discard - warned = false + warned = @suppress @databases.each do |db| next unless db.weakref_alive? @@ -49,6 +50,11 @@ def discard end @databases.clear end + + # Call to suppress the fork-related warnings. + def suppress_warnings! + @suppress = true + end end end end From 04d111c9651c2fd394094def9a7a33c09e03f8fa Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 24 Sep 2024 16:39:19 -0400 Subject: [PATCH 26/27] version bump to v2.1.0.rc3 --- CHANGELOG.md | 7 +++++++ lib/sqlite3/version.rb | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdac1d58..843d4bea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # sqlite3-ruby Changelog +## prerelease 2.1.0.rc3 / 2024-09-18 + +### Improved + +- Allow suppression of fork safety warnings. [#566] @flavorjones + + ## prerelease 2.1.0.rc2 / 2024-09-18 ### Improved diff --git a/lib/sqlite3/version.rb b/lib/sqlite3/version.rb index 099d14a2..c8119403 100644 --- a/lib/sqlite3/version.rb +++ b/lib/sqlite3/version.rb @@ -1,3 +1,3 @@ module SQLite3 - VERSION = "2.1.0.rc2" + VERSION = "2.1.0.rc3" end From 9a18cb9697d5bf6bdfd19e20e49934b0ce83a12a Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 24 Sep 2024 17:37:06 -0400 Subject: [PATCH 27/27] version bump to v2.1.0 also, some additional information in the CHANGELOG and README files. --- CHANGELOG.md | 25 ++++++++----------------- README.md | 2 +- lib/sqlite3/version.rb | 2 +- 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 843d4bea..90ca0ad6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,20 +1,6 @@ # sqlite3-ruby Changelog -## prerelease 2.1.0.rc3 / 2024-09-18 - -### Improved - -- Allow suppression of fork safety warnings. [#566] @flavorjones - - -## prerelease 2.1.0.rc2 / 2024-09-18 - -### Improved - -- Address a performance regression in 2.1.0.rc1. - - -## prerelease 2.1.0.rc1 / 2024-09-18 +## 2.1.0 / 2024-09-24 ### Ruby @@ -28,9 +14,9 @@ Sqlite itself is [not fork-safe](https://www.sqlite.org/howtocorrupt.html#_carry - 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 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 "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558] @flavorjones +See the README's "Fork Safety" section and `adr/2024-09-fork-safety.md` for more information. [#558, #565, #566] @flavorjones ### Improved @@ -39,6 +25,11 @@ See the README "Fork Safety" section and `adr/2024-09-fork-safety.md` for more i - 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/README.md b/README.md index feeb2d2a..b83dcf7e 100644 --- a/README.md +++ b/README.md @@ -160,7 +160,7 @@ To help protect users of this gem from accidental corruption due to this lack of 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. +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. diff --git a/lib/sqlite3/version.rb b/lib/sqlite3/version.rb index c8119403..21b0c51c 100644 --- a/lib/sqlite3/version.rb +++ b/lib/sqlite3/version.rb @@ -1,3 +1,3 @@ module SQLite3 - VERSION = "2.1.0.rc3" + VERSION = "2.1.0" end