Enable TestMethodsMutating #6215
Conversation
Make Dict::lookup check if the dictionary has been modified. This is a workaround for a possible segfault. See bpo-46615 for the original discussion in CPython.
WalkthroughReplaces unchecked access in Changes
Sequence DiagramsequenceDiagram
participant Caller as caller
participant Lookup as DictInner::lookup
participant Indices as inner.indices
Caller->>Lookup: start lookup
Lookup->>Lookup: capture original_size
loop iterate slots
Lookup->>Indices: indices.get(idx)
alt entry present
Indices-->>Lookup: Some(entry)
alt entry is FREE
Lookup->>Lookup: check size == original_size
alt size changed
Lookup-->>Caller: error "set changed size during iteration"
else size same
Lookup->>Lookup: insert FREE slot / continue search
end
else entry occupied
Lookup-->>Caller: return entry
end
else entry missing (None)
Indices-->>Lookup: None
Lookup-->>Caller: error "set changed size during iteration"
end
end
Note over Lookup: final size-check before returning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_set.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/dict_inner.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (runcargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
vm/src/dict_inner.rs
{vm,stdlib}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use RustPython macros (
pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
vm/src/dict_inner.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 (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
vm/src/dict_inner.rs
Outdated
| let index_entry_ptr = inner.indices.get(index_index); | ||
| if index_entry_ptr.is_none() { | ||
| // Dictionary was modified under our hands, see TestMethodsMutating. | ||
| continue 'outer; | ||
| } | ||
| let index_entry = *index_entry_ptr.unwrap(); |
There was a problem hiding this comment.
Incomplete fix: idxs must be reset when concurrent modification is detected.
The safety check correctly detects when the dictionary is modified concurrently (e.g., when user's __eq__ method clears the dictionary, shrinking it from size 16 to 8), but idxs retains its stale mask. When continuing the outer loop, line 580-582 will reuse the existing GenIndexes via get_or_insert_with, causing idxs.next() to keep generating out-of-bounds indices and triggering an infinite retry loop.
Apply this diff to reset idxs when concurrent modification is detected:
let index_entry_ptr = inner.indices.get(index_index);
if index_entry_ptr.is_none() {
// Dictionary was modified under our hands, see TestMethodsMutating.
+ idxs = None;
continue 'outer;
}
let index_entry = *index_entry_ptr.unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let index_entry_ptr = inner.indices.get(index_index); | |
| if index_entry_ptr.is_none() { | |
| // Dictionary was modified under our hands, see TestMethodsMutating. | |
| continue 'outer; | |
| } | |
| let index_entry = *index_entry_ptr.unwrap(); | |
| let index_entry_ptr = inner.indices.get(index_index); | |
| if index_entry_ptr.is_none() { | |
| // Dictionary was modified under our hands, see TestMethodsMutating. | |
| idxs = None; | |
| continue 'outer; | |
| } | |
| let index_entry = *index_entry_ptr.unwrap(); |
🤖 Prompt for AI Agents
In vm/src/dict_inner.rs around lines 585 to 590, when the
concurrent-modification check finds index_entry_ptr.is_none() you must reset the
cached GenIndexes (idxs) before continuing the 'outer loop so the next iteration
will recreate it with the current mask; modify the branch to clear/take the idxs
(set to None or take ownership) immediately prior to continue 'outer to avoid
reusing a stale mask that causes out-of-bounds indices and an infinite retry
loop.
|
I will investigate later this weekend. Locally (Linux -- rust docker container) the test passes: |
|
Thank you! |
vm/src/dict_inner.rs
Outdated
| // Safety: index_index is generated | ||
| inner.indices.get_unchecked(index_index) | ||
| }; | ||
| let index_entry_ptr = inner.indices.get(index_index); |
There was a problem hiding this comment.
Would something like this work?
let index_entry = match inner.indices.get(index_index) {
None => continue 'outer,
Some(v) => v,
};
|
Ok, only the Ubuntu is running the platform independent tests (based on the workflow file). And the test is flaky so that it crashed always the same in a single run on my computer. I let it run 1000 times yesterday. |
3 participants
Make Dict::lookup check if the dictionary has been modified. This is a workaround for a possible segfault. See bpo-46615 for the original discussion in CPython.
Summary by CodeRabbit