Fix sqlite connection reinitialization #6288
Conversation
WalkthroughRefactors Changes
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
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (5)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/stdlib/src/sqlite.rs (1)
906-906: Consider stronger memory ordering for the initialized flag.Using
Ordering::Relaxedfor theinitializedflag swap may not provide sufficient synchronization guarantees across threads. When one thread setsinitializedtotrue(line 934), another thread reading it with Relaxed ordering might not see the updated value promptly, or might observe it out of order with respect to other field updates.Consider using
Ordering::AcqRelfor the swap andOrdering::Releasewhen storing (line 934) to ensure proper synchronization.Apply this change for stronger guarantees:
- let was_initialized = zelf.initialized.swap(false, Ordering::Relaxed); + let was_initialized = zelf.initialized.swap(false, Ordering::AcqRel);And at line 934:
- zelf.initialized.store(true, Ordering::Relaxed); + zelf.initialized.store(true, Ordering::Release);
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_sqlite3/test_dbapi.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/stdlib/src/sqlite.rs(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (6)
crates/stdlib/src/sqlite.rs (6)
836-840: LGTM! Thread-safe field wrappers are appropriate.The atomic wrappers for
detect_typesandcheck_same_thread, plus the mutex forthread_ident, correctly enable thread-safe access to these fields that are read outside thedbmutex protection. Theinitializedflag properly tracks initialization state for re-initialization handling.
869-880: LGTM! Constructor initialization is correct.The initialization logic properly sets
initializedtotruefor base classes (wheredbis initialized immediately) andfalsefor subclasses (where__init__must be called). The use ofRadium::newfor atomic fields is appropriate.
905-936: Potential race condition with concurrent re-initialization.If two threads concurrently call
__init__on the same Connection object, they could interleave their operations unpredictably:
- Both threads swap
initializedto false- Both threads reset factories and drop the database
- Both threads open new databases
- Both threads try to store their database handle, with the second overwriting the first (leaking the first connection)
While concurrent
__init__calls are uncommon in typical Python usage, this is technically a data race. Consider documenting that__init__must not be called concurrently, or add protection against this scenario.
941-949: LGTM! Helper methods are correct.The
drop_dbandreset_factorieshelper methods correctly encapsulate their respective cleanup operations. Note thatclose()callsdrop_dbbut doesn't reset theinitializedflag, though this doesn't cause incorrect behavior since_db_lockchecksdb.is_some()rather than the flag directly.
1484-1495: LGTM! Atomic field access is correct.The
check_threadmethod correctly usesload()for the atomiccheck_same_threadfield andlock()for the mutex-protectedthread_ident. The Relaxed ordering is acceptable here since these are configuration checks.
1668-1669: LGTM! Atomic detect_types access is correct.All sites correctly read
detect_typesusingload(Ordering::Relaxed)from the atomic field. The Relaxed ordering is appropriate for reading this configuration field.Also applies to: 1717-1718, 1883-1884
|
Could you check this comment? #6288 (review) This sounds reasonable. And because it is initialization step, we don't have much disadvantage to use more stronger ordering. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/stdlib/src/sqlite.rs(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: auto_format
🔇 Additional comments (6)
crates/stdlib/src/sqlite.rs (6)
836-843: LGTM! Struct changes properly support reinitialization tracking.The addition of the
initializedflag and conversion of configuration fields to atomic types correctly enables tracking initialization state and thread-safe access. The use ofPyMutex<ThreadId>is appropriate given the TODO comment and platform constraints.
869-880: LGTM! Initialization correctly distinguishes base vs. subclass construction.The logic properly initializes the database immediately for the base
Connectionclass while deferring initialization for subclasses until__init__is called. The atomic field initialization usingRadium::newis appropriate.
941-949: LGTM! Helper methods correctly manage connection state.The
drop_dbandreset_factoriesmethods appropriately encapsulate state management. The use ofunsafeswap operations is consistent with the existing codebase patterns forPyAtomicRef.
1483-1495: LGTM! Thread checking correctly uses atomic flag and mutex.The implementation properly loads the
check_same_threadconfiguration flag atomically and locks the mutex to safely accessthread_ident. The use ofOrdering::Relaxedfor the configuration flag is acceptable since it doesn't require synchronization with other state.
1668-1669: LGTM! Consistent atomic loads for configuration values.The atomic loads of
detect_typeswithOrdering::Relaxedare appropriate throughout the codebase. Since this is a configuration value that doesn't require synchronization with other mutable state, relaxed ordering is sufficient and provides good performance.Also applies to: 1717-1718, 1883-1884
908-917: No action required—factory reset timing matches CPython behavior.CPython's sqlite3.Connection does reset row_factory and text_factory during init reinitialization. The code correctly resets factories before the database open attempt, aligning with CPython's reinitialization approach. CPython's own reinitialization is documented as having inconsistent timing between text_factory and row_factory, and the implementation here matches that behavior.
Apply code review feedback to use Ordering::AcqRel for the swap operation and Ordering::Release for the store operation to ensure proper synchronization across threads when setting the initialized flag.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/stdlib/src/sqlite.rs (2)
1483-1495: Thread safety check correctly updated to use atomic primitives.The method properly reads
check_same_threadatomically and uses the mutex forthread_ident. Whilecheck_same_threadusesRelaxedordering, this is acceptable because:
- The subsequent
db_lock()call provides additional synchronization via the database mutex- Thread checks happen before every operation, so any transient staleness during reinitialization is brief
- Reinitialization is rare (typically tests or error recovery)
If you want stricter guarantees, consider using
Acquireordering for thecheck_same_threadload to synchronize with theReleasestore ininitat line 934. However, this is optional since the current approach is likely sufficient for practical usage.- if self.check_same_thread.load(Ordering::Relaxed) { + if self.check_same_thread.load(Ordering::Acquire) {
1668-1669: LGTM:detect_typescorrectly read atomically in cursor operations.The consistent use of
Ordering::Relaxedfordetect_typesloads is acceptable because:
- These reads occur within operations that have already called
db_lock(), providing synchronization via the database mutex- Type detection behavior is not safety-critical
- Reinitialization-induced staleness would be transient and rare
Similar to
check_same_thread, you could optionally useAcquireordering for these loads to establish a stronger happens-before relationship with theReleasestore during initialization (line 934), but this is not necessary for correctness in typical usage patterns.Also applies to: 1717-1718, 1883-1884
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/stdlib/src/sqlite.rs(9 hunks)
🔇 Additional comments (5)
crates/stdlib/src/sqlite.rs (5)
834-843: LGTM: Struct fields correctly updated for thread-safe reinitialization.The addition of the
initializedatomic flag and conversion ofdetect_typesandcheck_same_threadto atomics enables proper state tracking during reinitialization. The TODO comment onthread_identis acknowledged in the PR comments.
854-883: LGTM: Proper initialization for both base and subclass Connection instances.The initialization correctly handles the base class (immediate DB setup) vs. subclass (deferred setup) distinction, and properly initializes all atomic fields.
902-937: LGTM: Reinitialization logic correctly implements CPython-compatible behavior.The initialization sequence properly handles the requirements from issue #6287:
- Atomically marks connection as uninitialized (line 906 with
AcqRelordering)- Resets factories to defaults (line 909), matching CPython behavior
- Opens DB before mutating other state (line 917), so failures leave the connection uninitialized and subsequent operations raise
ProgrammingError- Only marks initialized on success (line 934 with
Releaseordering)The memory ordering has been improved from the previous version, now using
AcqRelfor the swap andReleasefor the final store, which is stronger than the past review's suggestion and aligns with the maintainer's comment that "using stronger ordering has little downside" for initialization.Based on learnings from past review comments.
941-949: LGTM: Helper methods correctly encapsulate reinitialization concerns.
drop_dbandreset_factoriescleanly separate the resource cleanup and default restoration logic, matching CPython's connection reinitialization behavior.
1039-1043: LGTM: Refactored to use thedrop_dbhelper for consistency.
|
Code has been automatically formatted The code in this PR has been formatted using Triggered by commit: You may need to pull the latest changes before pushing again: git pull origin fixed-issue-6287 |
2 participants
Fixed #6287
reference
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.