Fix dict race condition by youknowone · Pull Request #6720 · RustPython/RustPython

youknowone

📝 Walkthrough

Walkthrough

Three new vectorized accessor methods are added to PyDict (keys_vec, values_vec, items_vec) to expose dictionary contents as atomic snapshots. Corresponding methods are implemented in the underlying DictInner and Dict generic types. The VM's element extraction logic is extended to handle iteration over dict view types.

Changes

Cohort / File(s)	Summary
PyDict Accessor Methods `crates/vm/src/builtins/dict.rs`	Added three new public methods: `keys_vec()`, `values_vec()`, and `items_vec()` to return thread-safe vectorized snapshots of dictionary keys, values, and key-value pairs respectively.
Generic Dict Container Methods `crates/vm/src/dict_inner.rs`	Added public methods to `DictInner<T>` and `Dict<T>` to expose `values()` and `items()` as vectors, enabling retrieval of all values and key-value pairs in insertion order from the underlying entries collection.
VM Element Extraction Logic `crates/vm/src/vm/mod.rs`	Extended `extract_elements_with` to support atomic iteration over dict_keys_type, dict_values_type, and dict_items_type by obtaining vectorized snapshots and mapping elements through the provided function. Dict items are converted to Python tuples before applying the function.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A dict reveals its treasures three,
Keys and values, wild and free!
Vectors gather what was locked,
Snapshots safe—our VM's stocked!
Thread-safe access, swift and clean,
Finest dict views ever seen! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fix dict race condition' is vague and does not clearly describe the actual changes, which add new vectorized accessor methods to PyDict for thread-safe element extraction in iteration loops.	Consider a more specific title like 'Add thread-safe dict accessor methods for iteration' that better reflects the main change of introducing keys_vec, values_vec, and items_vec methods.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

📜 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 7e47212 and 917c9f6.

📒 Files selected for processing (3)

crates/vm/src/builtins/dict.rs
crates/vm/src/dict_inner.rs
crates/vm/src/vm/mod.rs

🧰 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 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/vm/src/builtins/dict.rs
crates/vm/src/dict_inner.rs
crates/vm/src/vm/mod.rs

🧠 Learnings (1)

📚 Learning: 2025-12-27T14:03:49.034Z

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

crates/vm/src/vm/mod.rs

🧬 Code graph analysis (2)

crates/vm/src/dict_inner.rs (1)

crates/vm/src/builtins/dict.rs (2)

values (405-407)

items (410-412)

crates/vm/src/vm/mod.rs (2)

crates/vm/src/builtins/dict.rs (4)

dict (774-774)

keys (400-402)

values (405-407)

items (410-412)

crates/vm/src/dict_inner.rs (3)

keys (548-554)

values (556-562)

items (564-570)

🔇 Additional comments (5)

crates/vm/src/dict_inner.rs (1)

556-570: LGTM! Consistent implementation following established patterns.

The new values() and items() methods correctly mirror the existing keys() method pattern (lines 548-554): acquire read lock, filter-map entries, collect to Vec. This ensures atomic snapshots while the lock is held.

crates/vm/src/vm/mod.rs (3)

815-837: LGTM! Atomic snapshots for dict views prevent race conditions.

The implementation correctly uses the new keys_vec(), values_vec(), and items_vec() methods to get atomic snapshots before iteration, solving the race condition issue.

23-27: LGTM!

Import additions are correctly organized to support the new dict view type checks.

834-836: This optimization is not valid and would not compile.

The Context::new_tuple method requires Vec<PyObjectRef>, and arrays [T; N] do not implement IntoPyTuple. While VirtualMachine::new_tuple accepts impl IntoPyTuple, the code uses self.ctx.new_tuple(), which calls the Context version directly. The suggestion to change vec![k, v] to [k, v] would fail to compile.

Likely an incorrect or invalid review comment.

crates/vm/src/builtins/dict.rs (1)

65-81: LGTM! Well-documented atomic snapshot accessors.

The _vec suffix appropriately distinguishes these methods from the existing view-returning methods (lines 400-412). The doc comments clearly explain the thread-safety guarantees.

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