Fix sqlite connection reinitialization by ever0de · Pull Request #6288 · RustPython/RustPython

ever0de

Walkthrough

Refactors Connection in crates/stdlib/src/sqlite.rs to use concurrency-safe primitives: adds initialized: PyAtomic<bool>, converts detect_types, check_same_thread to atomics, changes thread_ident to a mutex, and updates init/reinit, close, and thread-checking flows plus helper functions drop_db and reset_factories.

Changes

Cohort / File(s)	Summary
SQLite Connection concurrency & init changes `crates/stdlib/src/sqlite.rs`	Added `initialized: PyAtomic<bool>`; converted `detect_types: c_int` → `PyAtomic<c_int>`, `check_same_thread: bool` → `PyAtomic<bool>`, and `thread_ident: ThreadId` → `PyMutex<ThreadId>`. Updated `py_new`/init and reinitialization flow (open DB before mutating state, ensure uninitialized on failure), added `drop_db` and `reset_factories`, and replaced direct field accesses with atomic loads/stores and mutex usage across connection, cursor, and statement code paths.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant C as Connection
    participant S as SQLiteDB
    participant A as Atomics

    U->>C: __init__(params)
    C->>A: initialized.store(false)
    C->>S: open database
    alt DB open succeeds
        C->>A: set detect_types / check_same_thread
        C->>C: set thread_ident (PyMutex)
        C->>A: initialized.store(true)
        C-->>U: return
    else DB open fails
        C->>A: ensure initialized stays false
        C-->>U: raise Error
    end

    U->>C: execute / executemany
    C->>A: if !initialized.load() -> raise ProgrammingError
    alt initialized == true
        C->>A: check_same_thread.load()
        C->>C: lock thread_ident if needed
        C->>S: execute statement
        C-->>U: result
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas needing extra attention:
- Atomic ordering and correctness for initialized, detect_types, and check_same_thread.
- Reinitialization failure paths ensuring initialized remains false on error.
- All call sites (cursor/statement/description) updated to use new atomics.
- thread_ident mutex use and potential contention/latency impacts.

Possibly related PRs

Fix sqlite Connection initialization check #6199 — modifies sqlite Connection initialization flow and DB lifecycle; closely related to the init/reinit changes.

Suggested reviewers

youknowone
ShaharNaveh

Poem

🐇 I hopped through atomics, soft and bright,

initialized set to guard the night,
mutex cradles the creator thread tight,
re-init falters — we keep state right,
now connections rest secure and light.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix sqlite connection reinitialization' directly and clearly summarizes the main change, addressing the core issue of handling state invalidation during failed Connection reinitialization.
Linked Issues check	✅ Passed	The implementation addresses all coding requirements from #6287: introduces initialized flag tracking via PyAtomic, ensures state is marked uninitialized on failed reinitialization, and supports proper ProgrammingError raising on subsequent operations.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing sqlite connection reinitialization: atomic state management, thread confinement, and initialization flow align with the issue requirements without introducing unrelated modifications.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ece048d and c9fc8a7.

📒 Files selected for processing (1)

crates/stdlib/src/sqlite.rs (9 hunks)

🔇 Additional comments (5)

crates/stdlib/src/sqlite.rs (5)

906-936: Reinitialization logic looks correct.

The initialization flow properly ensures that if initialize_db (line 917) fails, the connection remains in an uninitialized state (initialized=false, db=None), which will cause subsequent operations to raise ProgrammingError via _db_lock. This matches the PR objectives.

The memory ordering changes (AcqRel at line 906, Release at line 934) address the past review comment about establishing proper happens-before relationships.

Minor observation: The Release store at line 934 publishes all prior Relaxed stores (lines 926-930), but only threads that Acquire-load initialized would see these updates through that synchronization path. Since most code paths use the db mutex for synchronization instead, this is likely acceptable.

941-949: Helper methods look good.

The drop_db and reset_factories helpers correctly encapsulate resource cleanup and state reset. The reset_factories implementation matches CPython's behavior as noted in the comment at line 908.

1484-1495: Thread checking logic is correct.

The atomic load of check_same_thread with Relaxed ordering is appropriate since it's a boolean safety flag. The mutex lock on thread_ident properly serializes access to the thread ID.

1668-1669: Atomic reads of detect_types are appropriate.

Using Relaxed ordering for reading detect_types is acceptable here since:

It's a configuration parameter that affects parsing behavior, not safety

Even if a thread briefly sees a stale value during reinitialization, it won't cause corruption

The database mutex provides the primary synchronization for correctness

Also applies to: 1717-1718, 1883-1884

836-843: The initialized field is redundant; db.is_some() already guards operations.

The initialized flag tracks reinitialization state but is never checked to guard database operations. The actual guard is db.is_some() in the _db_lock method (line 969), which raises "Base Connection.init not called." when the connection is uninitialized.

If init() fails, both db and initialized remain false/None, so the behavior is correct. However, the initialized field appears unused for operation safety—consider removing it unless it's explicitly needed for other purposes (e.g., observability or future optimization noted in the TODO on thread_ident).

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}