diff --git a/CHANGELOG.md b/CHANGELOG.md index e91dbdac..9c584b9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,18 @@ ## next / unreleased +### 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. + +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..feeb2d2a 100644 --- a/README.md +++ b/README.md @@ -148,6 +148,23 @@ It is generally recommended that if applications want to share a database among threads, they _only_ share the database instance object. Other objects are fine to share, but may require manual locking for thread safety. + +## Fork Safety + +[Sqlite is not fork +safe](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_) +and instructs users to not carry an open writable database connection across a `fork()`. Using an inherited +connection in the child may corrupt your database, leak memory, or cause other undefined behavior. + +To help protect users of this gem from accidental corruption due to this lack of fork safety, the gem will immediately close any open writable databases in the child after a fork. Discarding writable +connections in the child will incur a small one-time memory leak per connection, but that's +preferable to potentially corrupting your database. + +Whenever possible, close writable connections in the parent before forking. + +See [./adr/2024-09-fork-safety.md](./adr/2024-09-fork-safety.md) for more information and context. + + ## Support ### Installation or database extensions diff --git a/adr/2024-09-fork-safety.md b/adr/2024-09-fork-safety.md new file mode 100644 index 00000000..b5b26c37 --- /dev/null +++ b/adr/2024-09-fork-safety.md @@ -0,0 +1,72 @@ + +# 2024-09 Automatically close database connections when carried across fork() + +## Status + +Accepted, but we can revisit more complex solutions if we learn something that indicates that effort is worth it. + + +## Context + +In August 2024, Andy Croll opened an issue[^issue] describing sqlite file corruption related to solid queue. After investigation, we were able to reproduce corruption under certain circumstances when forking a process with open sqlite databases.[^repro] + +SQLite is known to not be fork-safe[^howto], so this was not entirely surprising though it was the first time your author had personally seen corruption in the wild. The corruption became much more likely after the sqlite3-ruby gem improved its memory management with respect to open statements[^gemleak] in v2.0.0. + +Advice from upstream contributors[^advice] is, essentially: don't fork if you have open database connections. Or, if you have forked, don't call `sqlite3_close` on those connections and thereby leak some amount of memory in the child process. Neither of these options are ideal, see below. + + +## Decisions + +1. Open writable database connections carried across a `fork()` will automatically be closed in the child process to mitigate the risk of corrupting the database file. +2. These connections will be incompletely closed ("discarded") which will result in a one-time memory leak in the child process. + +First, the gem will register an "after fork" handler via `Process._fork` that will close any open writable database connections in the child process. This is a best-effort attempt to avoid corruption, but it is not guaranteed to prevent corruption in all cases. Any connections closed by this handler will also emit a warning to let users know what's happening. + +Second, the sqlite3-ruby gem will store the ID of the process that opened each database connection. If, when a writable database is closed (either explicitly with `Database#close` or implicitly via GC or after-fork callback) the current process ID is different from the original process, then we "discard" the connection. + +"Discard" here means: + +- `sqlite3_close_v2` is not called on the database, because it is unsafe to do so per sqlite instructions[^howto]. + - Open file descriptors associated with the database are closed. + - Any memory that can be freed safely is recovered. + - But some memory will be lost permanently (a one-time "memory leak"). +- The `Database` object acts "closed", including returning `true` from `#closed?`. +- Related `Statement` objects are rendered unusable and will raise an exception if used. + +Note that readonly databases are being treated as "fork safe" and are not affected by these changes. + + +## Consequences + +The positive consequence is that we remove a potential cause of database corruption for applications that fork with active sqlite database connections. + +The negative consequence is that, for each discarded connection, some memory will be permanently lost (leaked) in the child process. We consider this to be an acceptable tradeoff given the risk of data loss. + + +## Alternatives considered. + +### 1. Require applications to close database connections before forking. + +This is the advice[^advice] given by the upstream maintainers of sqlite, and so was the first thing we tried to implement in Rails in [rails/rails#52931](https://github.com/rails/rails/pull/52931)[^before_fork]. That first simple implementation was not thread safe, however, and in order to make it thread-safe it would be necessary to pause all sqlite database activity, close the open connections, and then fork. At least one Rails core team member was not happy that this would interfere with database connections in the parent, and the complexity of a thread-safe solution seemed high, so this work was paused. + +### 2. Memory arena + +Sqlite offers a configuration option to specify custom memory functions for malloc et al. It seems possible that the sqlite3-ruby gem could implement a custom arena that would be used by sqlite so that in a new process, after forking, all the memory underlying the sqlite Ruby objects could be discarded in a single operation. + +I think this approach is promising, but complex and risky. Sqlite is a complex library and uses shared memory in addition to the traditional heap. Would throwing away the heap memory (the arena) result in a segfault or other undefined behaviors or corruption? Determining the answer to that question feels expensive in and of itself, and any solution along these lines would not be supported by the sqlite authors. We can explore this space if the memory leak from discarded connections turns out to be a large source of pain. + + +## References + +- [Database connections carried across fork() will not be fully closed by flavorjones · Pull Request #558 · sparklemotion/sqlite3-ruby](https://github.com/sparklemotion/sqlite3-ruby/pull/558) + + +## Footnotes + +[^issue]: [SQLite queue database corruption · Issue #324 · rails/solid_queue](https://github.com/rails/solid_queue/issues/324) +[^repro]: [flavorjones/2024-09-13-sqlite-corruption: Temporary repo, reproduction of sqlite database corruption.](https://github.com/flavorjones/2024-09-13-sqlite-corruption) +[^howto]: [How To Corrupt An SQLite Database File: §2.6 Carrying an open database connection across a fork()](https://www.sqlite.org/howtocorrupt.html#_carrying_an_open_database_connection_across_a_fork_) +[^gemleak]: [Always call sqlite3_finalize in deallocate func by haileys · Pull Request #392 · sparklemotion/sqlite3-ruby](https://github.com/sparklemotion/sqlite3-ruby/pull/392) +[^advice]: [SQLite Forum: Correct way of carrying connections over forked processes](https://sqlite.org/forum/forumpost/1fa07728204567a0a136f442cb1c59e3117da96898b7fa3290b0063ae7f6f012) +[^before_fork]: [SQLite3Adapter: Ensure fork-safety by flavorjones · Pull Request #52931 · rails/rails](https://github.com/rails/rails/pull/52931#issuecomment-2351365601) +[^config]: [SQlite3 Configuration Options](https://www.sqlite.org/c3ref/c_config_covering_index_scan.html) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 2ad07de5..8e833faf 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -12,6 +12,63 @@ 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) +{ + if (ctx->db) { + int isReadonly = (ctx->flags & SQLITE_OPEN_READONLY); + + 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. + discard_db(ctx); + } + } +} + + static void database_mark(void *ctx) { @@ -22,11 +79,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 +105,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 +118,6 @@ utf16_string_value_ptr(VALUE str) return RSTRING_PTR(str); } -static VALUE sqlite3_rb_close(VALUE self); - sqlite3RubyPtr sqlite3_database_unwrap(VALUE database) { @@ -77,6 +131,7 @@ rb_sqlite3_open_v2(VALUE self, VALUE file, VALUE mode, VALUE zvfs) { sqlite3RubyPtr ctx; int status; + int flags; TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); @@ -89,14 +144,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; } @@ -119,21 +176,38 @@ rb_sqlite3_disable_quirk_mode(VALUE self) #endif } -/* call-seq: db.close +/* + * 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}[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. * - * Closes this database. + * See rdoc-ref:adr/2024-09-fork-safety.md for more information on fork safety. */ static VALUE sqlite3_rb_close(VALUE self) { sqlite3RubyPtr ctx; - sqlite3 *db; TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); - db = ctx->db; - CHECK(db, sqlite3_close_v2(ctx->db)); + close_or_discard_db(ctx); - ctx->db = NULL; + rb_iv_set(self, "-aggregators", Qnil); + + return self; +} + +/* private method, primarily for testing */ +static VALUE +sqlite3_rb_discard(VALUE self) +{ + sqlite3RubyPtr ctx; + TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); + + discard_db(ctx); rb_iv_set(self, "-aggregators", Qnil); @@ -871,6 +945,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); @@ -889,6 +967,7 @@ init_sqlite3_database(void) rb_define_private_method(cSqlite3Database, "open16", rb_sqlite3_open16, 1); rb_define_method(cSqlite3Database, "collation", collation, 2); rb_define_method(cSqlite3Database, "close", sqlite3_rb_close, 0); + rb_define_private_method(cSqlite3Database, "discard", sqlite3_rb_discard, 0); rb_define_method(cSqlite3Database, "closed?", closed_p, 0); rb_define_method(cSqlite3Database, "total_changes", total_changes, 0); rb_define_method(cSqlite3Database, "trace", trace, -1); diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h index 3123f4fe..1ef7b245 100644 --- a/ext/sqlite3/database.h +++ b/ext/sqlite3/database.h @@ -8,6 +8,8 @@ struct _sqlite3Ruby { VALUE busy_handler; int stmt_timeout; struct timespec stmt_deadline; + rb_pid_t owner; + int flags; }; typedef struct _sqlite3Ruby sqlite3Ruby; diff --git a/ext/sqlite3/extconf.rb b/ext/sqlite3/extconf.rb index c648d9e9..021b3304 100644 --- a/ext/sqlite3/extconf.rb +++ b/ext/sqlite3/extconf.rb @@ -131,6 +131,8 @@ def configure_extension end have_func("sqlite3_prepare_v2") + have_func("sqlite3_db_name", "sqlite3.h") # v3.39.0 + have_type("sqlite3_int64", "sqlite3.h") have_type("sqlite3_uint64", "sqlite3.h") end diff --git a/ext/sqlite3/statement.c b/ext/sqlite3/statement.c index cb65efb7..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/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..a39ce159 --- /dev/null +++ b/lib/sqlite3/fork_safety.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require "weakref" + +# based on Rails's active_support/fork_tracker.rb +module SQLite3 + module ForkSafety + module CoreExt + def _fork + pid = super + if pid == 0 + ForkSafety.discard + end + pid + end + end + + @databases = [] + @mutex = Mutex.new + + class << self + def hook! + ::Process.singleton_class.prepend(CoreExt) + end + + def track(database) + @mutex.synchronize do + @databases << WeakRef.new(database) + end + end + + def discard + warned = 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("Writable sqlite database connection(s) were inherited from a forked process. " \ + "This is unsafe and the connections are being closed to prevent possible data " \ + "corruption. Please close writable sqlite database connections before forking.", + uplevel: 0) + warned = true + end + db.close + end + end + @databases.clear + end + 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/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_discarding.rb b/test/test_discarding.rb new file mode 100644 index 00000000..20339ac1 --- /dev/null +++ b/test/test_discarding.rb @@ -0,0 +1,173 @@ +require_relative "helper" + +module SQLite3 + class TestDiscardDatabase < SQLite3::TestCase + DBPATH = "test.db" + + def setup + FileUtils.rm_f(DBPATH) + super + end + + def teardown + super + FileUtils.rm_f(DBPATH) + end + + def in_a_forked_process + @read, @write = IO.pipe + old_stderr, $stderr = $stderr, StringIO.new + + Process.fork do + @read.close + begin + yield @write + rescue => e + old_stderr.write("child exception: #{e.message}") + end + @write.write($stderr.string) + @write.close + exit! + end + + $stderr = old_stderr + @write.close + *@results = *@read.readlines + @read.close + end + + def test_fork_discards_an_open_readwrite_connection + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + skip("ruby 3.0 doesn't have Process._fork") if RUBY_VERSION < "3.1.0" + + GC.start + begin + db = SQLite3::Database.new(DBPATH) + + in_a_forked_process do |write| + write.write(db.closed? ? "ok\n" : "fail\n") + end + + assertion, *stderr = *@results + + assert_equal("ok", assertion.chomp, "closed? did not return true") + assert_equal(1, stderr.count, "unexpected output on stderr: #{stderr.inspect}") + assert_match( + /warning: Writable sqlite database connection\(s\) were inherited from a forked process/, + stderr.first, + "expected warning was not emitted" + ) + ensure + db&.close + end + end + + def test_fork_does_not_discard_closed_connections + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + db = SQLite3::Database.new(DBPATH) + db.close + + in_a_forked_process do |write| + write.write(db.closed? ? "ok\n" : "fail\n") + write.write($stderr.string) # should be empty write, no warnings emitted + write.write("done\n") + end + + assertion, *rest = *@results + + assert_equal("ok", assertion.chomp, "closed? did not return true") + assert_equal(1, rest.count, "unexpected output on stderr: #{rest.inspect}") + assert_equal("done", rest.first.chomp, "unexpected output on stderr: #{rest.inspect}") + ensure + db&.close + end + end + + def test_fork_does_not_discard_readonly_connections + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + SQLite3::Database.open(DBPATH) do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new(DBPATH, readonly: true) + + in_a_forked_process do |write| + write.write(db.closed? ? "fail\n" : "ok\n") # should be open and readable + write.write((db.execute("select * from foo") == [[1]]) ? "ok\n" : "fail\n") + write.write($stderr.string) # should be an empty write, no warnings emitted + write.write("done\n") + end + + assertion1, assertion2, *rest = *@results + + assert_equal("ok", assertion1.chomp, "closed? did not return false") + assert_equal("ok", assertion2.chomp, "could not read from database") + assert_equal(1, rest.count, "unexpected output on stderr: #{rest.inspect}") + assert_equal("done", rest.first.chomp, "unexpected output on stderr: #{rest.inspect}") + ensure + db&.close + end + end + + def test_close_does_not_discard_readonly_connections + skip("interpreter doesn't support fork") unless Process.respond_to?(:fork) + skip("valgrind doesn't handle forking") if i_am_running_in_valgrind + + GC.start + begin + SQLite3::Database.open(DBPATH) do |db| + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + end + + db = SQLite3::Database.new(DBPATH, readonly: true) + + in_a_forked_process do |write| + write.write(db.closed? ? "fail\n" : "ok\n") # should be open and readable + db.close + write.write($stderr.string) # should be an empty write, no warnings emitted + write.write("done\n") + end + + assertion, *rest = *@results + + assert_equal("ok", assertion.chomp, "closed? did not return false") + assert_equal(1, rest.count, "unexpected output on stderr: #{rest.inspect}") + assert_equal("done", rest.first.chomp, "unexpected output on stderr: #{rest.inspect}") + ensure + db&.close + end + end + + def test_a_discarded_connection_with_statements + skip("discard leaks memory") if i_am_running_in_valgrind + + begin + db = SQLite3::Database.new(DBPATH) + db.execute("create table foo (bar int)") + db.execute("insert into foo values (1)") + stmt = db.prepare("select * from foo") + + db.send(:discard) + + e = assert_raises(SQLite3::Exception) { stmt.execute } + assert_match(/cannot use a statement associated with a closed database/, e.message) + + assert_nothing_raised { stmt.close } + assert_predicate(stmt, :closed?) + ensure + db&.close + end + end + end +end diff --git a/test/test_resource_cleanup.rb b/test/test_resource_cleanup.rb index fa23a0ac..51a4b2cf 100644 --- a/test/test_resource_cleanup.rb +++ b/test/test_resource_cleanup.rb @@ -17,11 +17,31 @@ def test_cleanup_unclosed_statement_object end end + # # this leaks the result set # def test_cleanup_unclosed_resultset_object # db = SQLite3::Database.new(':memory:') # db.execute('create table foo(text BLOB)') # stmt = db.prepare('select * from foo') # stmt.execute # end + + # # this leaks the incompletely-closed connection + # def test_cleanup_discarded_connections + # FileUtils.rm_f "test.db" + # db = SQLite3::Database.new("test.db") + # db.execute("create table posts (title text)") + # db.execute("insert into posts (title) values ('hello')") + # db.close + # 100.times do + # db = SQLite3::Database.new("test.db") + # db.execute("select * from posts limit 1") + # stmt = db.prepare("select * from posts") + # stmt.execute + # stmt.close + # db.discard + # end + # ensure + # FileUtils.rm_f "test.db" + # end end end