Skip to content

Commit

Permalink
Database#close invokes discard if the pid has changed
Browse files Browse the repository at this point in the history
This will help prevent corrupting the database.

Also provide a warning to let people know they're doing something
that's not supported by sqlite.
  • Loading branch information
flavorjones committed Sep 16, 2024
1 parent 4f0ab9f commit 760d766
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 15 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## next / unreleased

### Added

- New method `Database#discard` is intended to be used after forking a process with an open database connection. Forking is not supported by sqlite, and as an alternativ `discard` will "close" the connection and release what resources can be safely released. [#558] @flavorjones
- `Database#close` will detect if it's being closed from a child process, and if so will emit a warning and invoke `discard` instead. [#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
Expand Down
18 changes: 7 additions & 11 deletions ext/sqlite3/database.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ static void
deallocate(void *ctx)
{
sqlite3RubyPtr c = (sqlite3RubyPtr)ctx;
sqlite3 *db = c->db;
sqlite3 *db = c->db;

if (db) { sqlite3_close_v2(db); }
xfree(c);
Expand Down Expand Up @@ -62,8 +62,6 @@ utf16_string_value_ptr(VALUE str)
return RSTRING_PTR(str);
}

static VALUE sqlite3_rb_close(VALUE self);

sqlite3RubyPtr
sqlite3_database_unwrap(VALUE database)
{
Expand Down Expand Up @@ -119,18 +117,16 @@ rb_sqlite3_disable_quirk_mode(VALUE self)
#endif
}

/* call-seq: db.close
*
* Closes this database.
*/
static VALUE
sqlite3_rb_close(VALUE self)
sqlite3_rb__close(VALUE self)
{
sqlite3RubyPtr ctx;
TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx);

CHECK(ctx->db, sqlite3_close_v2(ctx->db));
ctx->db = NULL;
if (ctx->db) {
CHECK(ctx->db, sqlite3_close_v2(ctx->db));
ctx->db = NULL;
}

rb_iv_set(self, "-aggregators", Qnil);

Expand Down Expand Up @@ -921,7 +917,7 @@ init_sqlite3_database(void)
rb_define_private_method(cSqlite3Database, "open_v2", rb_sqlite3_open_v2, 3);
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, "_close", sqlite3_rb__close, 0);
rb_define_method(cSqlite3Database, "discard", sqlite3_rb_discard, 0);
rb_define_method(cSqlite3Database, "closed?", closed_p, 0);
rb_define_method(cSqlite3Database, "total_changes", total_changes, 0);
Expand Down
19 changes: 19 additions & 0 deletions lib/sqlite3/database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ def initialize file, options = {}, zvfs = nil
@results_as_hash = options[:results_as_hash]
@readonly = mode & Constants::Open::READONLY != 0
@default_transaction_mode = options[:default_transaction_mode] || :deferred
@owner_pid = Process.pid

if block_given?
begin
Expand All @@ -143,6 +144,24 @@ def initialize file, options = {}, zvfs = nil
end
end

# 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.
#
# See adr/2024-09-fork-safety.md for more information on fork safety.
def close
if Process.pid != @owner_pid
warn "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.", uplevel: 1
discard
else
_close
end
end

# call-seq: db.encoding
#
# Fetch the encoding set on this database
Expand Down
4 changes: 0 additions & 4 deletions test/helper.rb
Original file line number Diff line number Diff line change
@@ -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}"
Expand Down
21 changes: 21 additions & 0 deletions test/test_database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,27 @@ def test_discard_a_connection
end
end

def test_close_in_a_new_process_calls_discard_and_warns
db = SQLite3::Database.new("test.db")

assert_equal(Process.pid, db.instance_variable_get(:@owner_pid))

called = false
db.define_singleton_method(:discard) do
called = true
end
db.instance_variable_set(:@owner_pid, 1)

assert_output(nil, /warning: An open sqlite database connection was inherited from a forked process/) do
db.close
end
assert(called)
ensure
db.instance_variable_set(:@owner_pid, Process.pid)
db.close
FileUtils.rm_f("test.db")
end

def test_discard_a_closed_connection
db = SQLite3::Database.new("test.db")
db.close
Expand Down

0 comments on commit 760d766

Please sign in to comment.