Clean up docs, move discard tests to their own file

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.
 
 

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

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 <tt>fork()</tt> 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)

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

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