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