8000 Bump libsqlite3-sys from 0.28 to 0.36 by ever0de · Pull Request #6472 · RustPython/RustPython · GitHub
[go: up one dir, main page]
Skip to content

Bump libsqlite3-sys from 0.28 to 0.36 #6472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Dec 24, 2025
Merged

Bump libsqlite3-sys from 0.28 to 0.36 #6472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5008c83
Bump libsqlite3-sys from 0.28 to 0.36
ever0de Dec 23, 2025
6798c26
Fix clippy
ever0de Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/stdlib/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ oid-registry = { version = "0.8", features = ["x509", "pkcs1", "nist_algs"], opt
pkcs8 = { version = "0.10", features = ["encryption", "pkcs5", "pem"], optional = true }

[target.'cfg(not(any(target_os = "android", target_arch = "wasm32")))'.dependencies]
libsqlite3-sys = { version = "0.28", features = ["bundled"], optional = true }
libsqlite3-sys = { version = "0.36", features = ["bundled"], optional = true }
lzma-sys = "0.1"
xz2 = "0.1"

Expand Down
8 changes: 4 additions & 4 deletions crates/stdlib/src/sqlite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ mod _sqlite {
sqlite3_bind_null, sqlite3_bind_parameter_count, sqlite3_bind_parameter_name,
sqlite3_bind_text, sqlite3_blob, sqlite3_blob_bytes, sqlite3_blob_close, sqlite3_blob_open,
sqlite3_blob_read, sqlite3_blob_write, sqlite3_busy_timeout, sqlite3_changes,
sqlite3_close_v2, sqlite3_column_blob, sqlite3_column_bytes, sqlite3_column_count,
sqlite3_close, sqlite3_column_blob, sqlite3_column_bytes, sqlite3_column_count,
sqlite3_column_decltype, sqlite3_column_double, sqlite3_column_int64, sqlite3_column_name,
sqlite3_column_text, sqlite3_column_type, sqlite3_complete, sqlite3_context,
sqlite3_context_db_handle, sqlite3_create_collation_v2, sqlite3_create_function_v2,
Expand Down Expand Up @@ -1349,14 +1349,14 @@ mod _sqlite {
fn set_trace_callback(&self, callable: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
let db = self.db_lock(vm)?;
let Some(data) = CallbackData::new(callable, vm) else {
unsafe { sqlite3_trace_v2(db.db, SQLITE_TRACE_STMT as u32, None, null_mut()) };
unsafe { sqlite3_trace_v2(db.db, SQLITE_TRACE_STMT, None, null_mut()) };
return Ok(());
};

let ret = unsafe {
sqlite3_trace_v2(
db.db,
SQLITE_TRACE_STMT as u32,
SQLITE_TRACE_STMT,
Some(CallbackData::trace_callback),
Box::into_raw(Box::new(data)).cast(),
)
Expand Down Expand Up @@ -2661,7 +2661,7 @@ mod _sqlite {

impl Drop for Sqlite {
fn drop(&mut self) {
unsafe { sqlite3_close_v2(self.raw.db) };
unsafe { sqlite3_close(self.raw.db) };
}
Comment on lines 2662 to 2665
Copy link
Contributor
@coderabbitai coderabbitai bot Dec 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for the Drop implementation and statement cleanup patterns

echo "=== Current Drop implementation for Sqlite ==="
rg -n -A5 'impl Drop for Sqlite' crates/stdlib/src/sqlite.rs

echo -e "\n=== Finding Statement creation and lifecycle management ==="
rg -n -B3 -A5 'struct SqliteStatement|fn new.*Statement' crates/stdlib/src/sqlite.rs | head -80

echo -e "\n=== Finding drop_db calls and connection closure ==="
rg -n -B2 -A8 'fn drop_db' crates/stdlib/src/sqlite.rs

echo -e "\n=== Checking cursor statement cleanup ==="
rg -n -B3 -A10 'impl Drop for Cursor' crates/stdlib/src/sqlite.rs

echo -e "\n=== Searching for Cursor close methods ==="
rg -n -B2 -A8 'fn close\(' crates/stdlib/src/sqlite.rs | head -80

Repository: RustPython/RustPython

Length of output: 2855

🏁 Script executed:

#!/bin/bash
# Search for Cursor struct definition and statement storage

echo "=== Finding Cursor struct definition ==="
rg -n -B5 -A20 'struct Cursor' crates/stdlib/src/sqlite.rs | head -100

echo -e "\n=== Finding how statements are stored/managed ==="
rg -n -B3 -A3 'description:|\.stmt|self\.stmt' crates/stdlib/src/sqlite.rs | head -100

echo -e "\n=== Finding execute methods that create statements ==="
rg -n -B2 -A15 'fn execute\(' crates/stdlib/src/sqlite.rs | head -150

echo -e "\n=== Search for statement finalization patterns ==="
rg -n 'sqlite3_finalize|finalize_stmt' crates/stdlib/src/sqlite.rs

Repository: RustPython/RustPython

Length of output: 3836

🏁 Script executed:

#!/bin/bash
# Get the complete Cursor.close() implementation
echo "=== Cursor close method complete implementation ==="
sed -n '1822,1860p' crates/stdlib/src/sqlite.rs

echo -e "\n=== Connection.drop_db() and its usage ==="
sed -n '941,950p' crates/stdlib/src/sqlite.rs

echo -e "\n=== Check if statements are cleaned up before drop_db ==="
rg -n -B10 'drop_db' crates/stdlib/src/sqlite.rs | grep -A10 -B10 'close\|drop_db'

echo -e "\n=== Check what Cursor has when close is called ==="
sed -n '1820,1860p' crates/stdlib/src/sqlite.rs

Repository: RustPython/RustPython

Length of output: 4200

🏁 Script executed:

#!/bin/bash
# Find Statement struct definition
echo "=== Finding Statement struct definition ==="
rg -n -B3 -A15 'struct Statement' crates/stdlib/src/sqlite.rs

echo -e "\n=== Check Statement::new implementation ==="
rg -n -B2 -A20 'impl Statement' crates/stdlib/src/sqlite.rs | head -100

echo -e "\n=== Verify if Statement holds connection reference ==="
rg -n 'struct Statement \{' -A20 crates/stdlib/src/sqlite.rs

Repository: RustPython/RustPython

Length of output: 2293

Ensure all cursors are closed before closing the connection, or use sqlite3_close_v2 to defer close until statements finalize.

The change from sqlite3_close_v2 to sqlite3_close introduces a resource leak risk:

  • sqlite3_close returns SQLITE_BUSY if unfinalized statements exist and fails silently in Drop
  • Connection.close() calls drop_db() immediately while active Cursor objects may still hold prepared statements
  • Statements only finalize when their owning Cursor is dropped, which may happen after the connection is dropped

Either:

  1. Ensure statements are explicitly finalized before Connection.close() is called
  2. Revert to sqlite3_close_v2 for more resilient cleanup semantics
  3. Add explicit statement cleanup in drop_db() before closing the connection

Copy link
Member
@youknowone youknowone Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ever0de Is this looking a correct review?

Copy link
Contributor
@coderabbitai coderabbitai bot Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author
@ever0de ever0de Dec 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created comprehensive tests comparing CPython's behavior with RustPython when closing connections with active cursors. Both implementations behave identically - they successfully close the connection even when cursors remain active, and subsequent cursor operations properly raise ProgrammingError.

Investigation of sqlite3_close vs sqlite3_close_v2:
While CPython does use sqlite3_close_v2() for its deferred cleanup semantics, libsqlite3-sys intentionally removed sqlite3_close_v2 from its API in version 0.29.0 (rusqlite/rusqlite@4390720).

The commit message explicitly states:

Omit API

sqlite3_close_v2 (for gced languages)

The rationale is that sqlite3_close_v2 is specifically designed for garbage-collected languages where destructor order is non-deterministic. In Rust, with its deterministic RAII-based resource management, the v2 variant is unnecessary. The library maintainers deliberately chose to expose only sqlite3_close as the canonical close function for Rust applications.

This means the use of sqlite3_close() in this PR is not only necessary due to API constraints but also aligns with rusqlite's design philosophy.

Why it works safely:
Our implementation is safe because:

  1. The drop_db() method takes ownership via .take(), ensuring clean Drop trait execution
  2. Statement finalization happens through RAII when SqliteStatement is dropped
  3. The existing test suite (including the new tests I created) confirms proper cleanup behavior
  4. Rust's ownership system guarantees deterministic cleanup order, unlike garbage-collected languages

The current implementation matches CPython's observable behavior even though the internal mechanism differs due to library design decisions.

Copy link
Member
@youknowone youknowone Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for confirming!

}

Expand Down
Loading
0