refactor(sqlite): add check_cursor_valid method for cursor state validation #6342

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

Jump to bottom

Merged

youknowone merged 1 commit into RustPython:main from ever0de:refactor/cursor-check-valid

Dec 8, 2025

Contributor

ever0de commented Dec 8, 2025 •

edited by coderabbitai bot

Loading

Extract cursor validity check into a separate method that doesn't hold the lock. This is useful for executescript which only needs to verify the cursor state but doesn't need to modify CursorInner.

Add check_cursor_valid() method that checks if cursor is initialized and not closed without retaining the mutex guard
Use check_cursor_valid() in executescript instead of inner() since executescript doesn't modify CursorInner

Summary by CodeRabbit

Refactor
- Consolidated repeated cursor-state checks into shared validation helpers to reduce duplication and improve maintainability.
- Added a non-mutating validation path so callers can verify cursor state without acquiring mutation locks.
- Script execution now validates cursor state without cloning internal state.
- Existing behavior for closed or uninitialized cursors and public APIs remain unchanged.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Contributor

coderabbitai bot commented Dec 8, 2025 •

edited

Loading

Walkthrough

Extracts cursor state checks in the SQLite module into helper functions (check_cursor_state and check_cursor_valid) and updates inner() and executescript() to use them, consolidating closed/uninitialized cursor validation without changing public APIs.

Changes

Cohort / File(s)	Summary
SQLite cursor helpers & callsites `crates/stdlib/src/sqlite.rs`	Adds `check_cursor_state(inner, vm)` to centralize closed/uninitialized checks, adds `check_cursor_valid(&self, vm)` for non-mutating validation, refactors `inner()` to call the helper (preserving `PyMappedMutexGuard` behavior), and updates `executescript()` to use `check_cursor_valid()` instead of cloning/checking `inner`. No public signatures changed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Single-file, localized refactor consolidating duplicated checks.
Pay attention to exact error messages/variants for closed vs uninitialized cursor and to correct usage of PyMappedMutexGuard mapping.

Possibly related PRs

fix(sqlite): Raise ProgrammingError when operating on a closed cursor #6339 — Similar centralization of sqlite cursor state checks and adjustments to inner()/executescript() handling.
Fix sqlite Cursor initialization check #6198 — Prior work addressing Cursor state validation and closed/uninitialized handling in sqlite code.

Suggested reviewers

youknowone
ShaharNaveh

Poem

🐇 I hopped through code to tidy the trail,
Reused checks now steady and pale,
Fewer repeats, the logic's right,
Cursors rest without a fight,
Hops and merges — what a tale! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: adding a check_cursor_valid method for cursor state validation, which aligns with the primary refactoring objective to extract cursor validity checks.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✨ 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 d07bb03 and 2f6c11e.

📒 Files selected for processing (1)

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

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/stdlib/src/sqlite.rs

⏰ 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). (11)

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 tests under miri
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (2)

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

1592-1619: Cursor state helper and lock usage look correct and keep semantics intact

Centralizing the “closed vs uninitialized” logic in check_cursor_state and reusing it from both inner() and check_cursor_valid() keeps the error messages consistent and maintains the existing invariants (inner is Some whenever you hit the unwrap_unchecked). check_cursor_valid() correctly acquires the mutex only for the duration of the state check and doesn’t retain the guard, which matches the intended use for read‑only validation. I don’t see correctness or concurrency regressions here.

1777-1784: Using check_cursor_valid in executescript matches the new helper’s intent

Swapping inner() for zelf.check_cursor_valid(vm)?; is appropriate here: executescript doesn’t touch CursorInner at all, it only needs to reject closed/uninitialized cursors up front and then operates solely on the underlying Connection. This avoids holding the cursor mutex across the entire script execution while preserving the previous ProgrammingError behavior.

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.}

coderabbitai bot reviewed

View reviewed changes

Contributor

coderabbitai bot left a comment

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

1592-1627: Centralized cursor state checking looks correct; minor optional API tweak

The extraction into cursor_state_error and reuse in both inner and check_cursor_valid correctly preserves existing error conditions for closed and uninitialized cursors while reducing duplication. The short‑lived mutex guard in check_cursor_valid achieves the goal of not retaining the lock across later work.

If you want to simplify the helper further, you could consider returning PyResult<()> from cursor_state_error and using ? at call sites instead of Option<PyBaseExceptionRef>, but this is purely stylistic.

📜 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 42d497a and 89d1bdc.

📒 Files selected for processing (1)

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

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/stdlib/src/sqlite.rs

⏰ 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). (11)

GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check the WASM package and demo
GitHub Check: Run tests under miri
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 (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Check Rust code with clippy
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (1)

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

1785-1792: executescript no longer holds the cursor mutex for the whole script execution

Switching executescript to call check_cursor_valid(vm)? instead of relying on inner(vm)? means the cursor’s state is validated, but the cursor mutex is not held while running the prepare/step loop on the connection. This aligns with the PR objective and should reduce unnecessary lock contention without changing the observable error behavior for closed or uninitialized cursors.

ever0de force-pushed the refactor/cursor-check-valid branch from 89d1bdc to d07bb03 Compare

December 8, 2025 08:12

coderabbitai bot reviewed

View reviewed changes

Contributor

coderabbitai bot left a comment

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

1592-1619: Nice consolidation of cursor state checks; minor doc nit only

The new check_cursor_state, updated inner, and check_cursor_valid nicely centralize the “closed/uninitialized” logic and keep the existing error messages/behavior while avoiding holding the cursor mutex longer than necessary. The unwrap_unchecked remains justified by the prior state check.

One tiny suggestion: the doc comment on check_cursor_valid says “without holding the lock”, but the method does acquire the lock briefly and just doesn’t retain the guard. Consider rephrasing to “without retaining the lock/guard” to avoid confusion for future readers.

📜 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 89d1bdc and d07bb03.

📒 Files selected for processing (1)

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

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/stdlib/src/sqlite.rs

⏰ 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). (11)

GitHub Check: Run tests under miri
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
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 snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Ensure compilation on various targets
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (windows-latest)

🔇 Additional comments (1)

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

1778-1799: Using check_cursor_valid in executescript achieves the intended lock‑lifetime reduction

Switching executescript to call zelf.check_cursor_valid(vm)? before operating on the connection keeps the existing cursor‑state validation (closed / uninitialized) but stops holding the cursor’s inner mutex across db_lock, sql_limit, and statement execution. This is a good refactor for reducing lock contention without changing observable behavior.


          refactor(sqlite): add check_cursor_valid method for cursor state vali…

2f6c11e

…dation

Extract cursor validity check into a separate method that doesn't hold
the lock. This is useful for executescript which only needs to verify
the cursor state but doesn't need to modify CursorInner.

- Add check_cursor_valid() method that checks if cursor is initialized
  and not closed without retaining the mutex guard
- Use check_cursor_valid() in executescript instead of inner() since
  executescript doesn't modify CursorInner

ever0de force-pushed the refactor/cursor-check-valid branch from d07bb03 to 2f6c11e Compare

December 8, 2025 08:17

youknowone approved these changes

View reviewed changes

Member

youknowone left a comment

👍

youknowone merged commit 7250b1f into RustPython:main

13 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet