Skip to content

Fix list.__repr__ deadlock #7275

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

Merged
youknowone merged 2 commits into from
Mar 1, 2026
Merged

Fix list.__repr__ deadlock #7275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff42ed6
Fix list repr deadlock
ShaharNaveh Feb 28, 2026
f9956b2
Unmark apssing test
ShaharNaveh Feb 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Lib/test/test_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ def test_list_resize_overflow(self):
with self.assertRaises((MemoryError, OverflowError)):
lst *= size

@unittest.skip("TODO: RUSTPYTHON; hangs")
def test_repr_mutate(self):
class Obj:
@staticmethod
Expand Down
37 changes: 32 additions & 5 deletions crates/vm/src/builtins/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ use crate::{
AsMapping, AsSequence, Comparable, Constructor, Initializer, IterNext, Iterable,
PyComparisonOp, Representable, SelfIter,
},
utils::collection_repr,
vm::VirtualMachine,
};
use rustpython_common::wtf8::Wtf8Buf;

use alloc::fmt;
use core::ops::DerefMut;

Expand Down Expand Up @@ -521,14 +522,40 @@ impl Representable for PyList {
if zelf.__len__() == 0 {
return Ok(vm.ctx.intern_str("[]").to_owned());
}

if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) {
// Clone elements before calling repr to release the read lock.
// Element repr may mutate the list (e.g., list.clear()), which
// needs a write lock and would deadlock if read lock is held.
let elements: Vec<PyObjectRef> = zelf.borrow_vec().to_vec();
Ok(vm
.ctx
.new_str(collection_repr(None, "[", "]", elements.iter(), vm)?))
let mut writer = Wtf8Buf::new();
writer.push_char('[');

let mut elements = zelf.borrow_vec().to_vec();
let mut size = zelf.__len__();
let mut first = true;
let mut i = 0;
while i < size {
if elements.len() != size {
// `repr` mutated the list. refetch it.
elements = zelf.borrow_vec().to_vec();
}

let item = &elements[i];

if first {
first = false;
} else {
writer.push_str(", ");
}

writer.push_wtf8(item.repr(vm)?.as_wtf8());

size = zelf.__len__(); // Refetch list size as `repr` may mutate the list.
i += 1;
}
Comment on lines +533 to +555
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Refresh elements by index each iteration, not only on length changes.

The current logic only refetches when length changes. If an element __repr__ mutates list contents without changing length, repr can emit stale items (snapshot values), which is a correctness mismatch.

Suggested fix 
-            let mut elements = zelf.borrow_vec().to_vec();
-            let mut size = zelf.__len__();
-            let mut first = true;
-            let mut i = 0;
-            while i < size {
-                if elements.len() != size {
-                    // `repr` mutated the list. refetch it.
-                    elements = zelf.borrow_vec().to_vec();
-                }
-
-                let item = &elements[i];
-
-                if first {
-                    first = false;
-                } else {
-                    writer.push_str(", ");
-                }
-
-                writer.push_wtf8(item.repr(vm)?.as_wtf8());
-
-                size = zelf.__len__(); // Refetch list size as `repr` may mutate the list.
-                i += 1;
-            }
+            let mut i = 0;
+            while let Some(item) = {
+                let elements = zelf.borrow_vec();
+                elements.get(i).cloned()
+            } {
+                if i != 0 {
+                    writer.push_str(", ");
+                }
+                writer.push_wtf8(item.repr(vm)?.as_wtf8());
+                i += 1;
+            }
📝 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.

Suggested change
let mut elements = zelf.borrow_vec().to_vec();
let mut size = zelf.__len__();
let mut first = true;
let mut i = 0;
while i < size {
if elements.len() != size {
// `repr` mutated the list. refetch it.
elements = zelf.borrow_vec().to_vec();
}
let item = &elements[i];
if first {
first = false;
} else {
writer.push_str(", ");
}
writer.push_wtf8(item.repr(vm)?.as_wtf8());
size = zelf.__len__(); // Refetch list size as `repr` may mutate the list.
i += 1;
}
let mut i = 0;
while let Some(item) = {
let elements = zelf.borrow_vec();
elements.get(i).cloned()
} {
if i != 0 {
writer.push_str(", ");
}
writer.push_wtf8(item.repr(vm)?.as_wtf8());
i += 1;
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/builtins/list.rs` around lines 533 - 555, The loop captures a
snapshot in elements and only refreshes it when length changes, which yields
stale items if an element's __repr__ mutates contents without resizing; update
the loop to re-fetch the current element by index on every iteration instead of
indexing into the stale elements snapshot (use zelf.borrow_vec() each iteration
or fetch a cloned element via zelf.borrow_vec().get(i).cloned().unwrap() before
calling item.repr(vm)), and keep using zelf.__len__() to track size changes;
adjust variables: elements, item, i, and the repr call (item.repr(vm)) so the
borrow does not persist across the repr call.


writer.push_char(']');
Ok(vm.ctx.new_str(writer))
} else {
Ok(vm.ctx.intern_str("[...]").to_owned())
}
Expand Down
Loading