From 1cd3d0395149e1da64e3c95082cc438b16cc0a5f Mon Sep 17 00:00:00 2001 From: Kangzhi Shi Date: Thu, 23 Sep 2021 11:27:03 +0200 Subject: [PATCH] fix deadlock and clear up --- vm/src/builtins/dict.rs | 42 ++++++++++++++++++++++++----------------- vm/src/builtins/iter.rs | 7 +++++-- vm/src/builtins/set.rs | 41 ++++++++++------------------------------ vm/src/dictdatatype.rs | 16 ++++++++-------- 4 files changed, 48 insertions(+), 58 deletions(-) diff --git a/vm/src/builtins/dict.rs b/vm/src/builtins/dict.rs index 39a6120660..d4ff3bd86a 100644 --- a/vm/src/builtins/dict.rs +++ b/vm/src/builtins/dict.rs @@ -587,7 +587,9 @@ impl Iterator for DictIter { type Item = (PyObjectRef, PyObjectRef); fn next(&mut self) -> Option { - self.dict.entries.next_entry(&mut self.position) + let (position, key, value) = self.dict.entries.next_entry(self.position)?; + self.position = position; + Some((key, value)) } fn size_hint(&self) -> (usize, Option) { @@ -735,20 +737,21 @@ macro_rules! dict_iterator { impl SlotIterator for $iter_name { #[allow(clippy::redundant_closure_call)] fn next(zelf: &PyRef, vm: &VirtualMachine) -> PyResult { - let mut status = PyRwLockWriteGuard::map(zelf.internal.write(), |x| &mut x.status); - let mut position = - PyRwLockWriteGuard::map(zelf.internal.write(), |x| &mut x.position); - if let IterStatus::Active(dict) = &*status { + let mut internal = zelf.internal.write(); + if let IterStatus::Active(dict) = &internal.status { if dict.entries.has_changed_size(&zelf.size) { - *status = IterStatus::Exhausted; + internal.status = IterStatus::Exhausted; return Err(vm.new_runtime_error( "dictionary changed size during iteration".to_owned(), )); } - match dict.entries.next_entry(&mut *position) { - Some((key, value)) => Ok(($result_fn)(vm, key, value)), + match dict.entries.next_entry(internal.position) { + Some((position, key, value)) => { + internal.position = position; + Ok(($result_fn)(vm, key, value)) + } None => { - *status = IterStatus::Exhausted; + internal.status = IterStatus::Exhausted; Err(vm.new_stop_iteration()) } } @@ -794,20 +797,25 @@ macro_rules! dict_iterator { impl SlotIterator for $reverse_iter_name { #[allow(clippy::redundant_closure_call)] fn next(zelf: &PyRef, vm: &VirtualMachine) -> PyResult { - let mut status = PyRwLockWriteGuard::map(zelf.internal.write(), |x| &mut x.status); - let mut position = - PyRwLockWriteGuard::map(zelf.internal.write(), |x| &mut x.position); - if let IterStatus::Active(dict) = &*status { + let mut internal = zelf.internal.write(); + if let IterStatus::Active(dict) = &internal.status { if dict.entries.has_changed_size(&zelf.size) { - *status = IterStatus::Exhausted; + internal.status = IterStatus::Exhausted; return Err(vm.new_runtime_error( "dictionary changed size during iteration".to_owned(), )); } - match dict.entries.prev_entry(&mut *position) { - Some((key, value)) => Ok(($result_fn)(vm, key, value)), + match dict.entries.prev_entry(internal.position) { + Some((position, key, value)) => { + if internal.position == position { + internal.status = IterStatus::Exhausted; + } else { + internal.position = position; + } + Ok(($result_fn)(vm, key, value)) + } None => { - *status = IterStatus::Exhausted; + internal.status = IterStatus::Exhausted; Err(vm.new_stop_iteration()) } } diff --git a/vm/src/builtins/iter.rs b/vm/src/builtins/iter.rs index caaed7dcec..b9e53f62c2 100644 --- a/vm/src/builtins/iter.rs +++ b/vm/src/builtins/iter.rs @@ -240,10 +240,13 @@ impl PyCallableIterator { impl IteratorIterable for PyCallableIterator {} impl SlotIterator for PyCallableIterator { fn next(zelf: &PyRef, vm: &VirtualMachine) -> PyResult { - if let IterStatus::Active(callable) = &*zelf.status.read() { + // let mut status = zelf.status.write(); + let status = zelf.status.upgradable_read(); + if let IterStatus::Active(callable) = &*status { let ret = callable.invoke((), vm)?; if vm.bool_eq(&ret, &zelf.sentinel)? { - *zelf.status.write() = IterStatus::Exhausted; + // *status = IterStatus::Exhausted; + *PyRwLockUpgradableReadGuard::upgrade(status) = IterStatus::Exhausted; Err(vm.new_stop_iteration()) } else { Ok(ret) diff --git a/vm/src/builtins/set.rs b/vm/src/builtins/set.rs index 7bcaa7988f..e572691656 100644 --- a/vm/src/builtins/set.rs +++ b/vm/src/builtins/set.rs @@ -16,7 +16,7 @@ use crate::{ IdProtocol, PyClassImpl, PyComparisonValue, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, TypeProtocol, }; -use rustpython_common::lock::{PyRwLock, PyRwLockWriteGuard}; +use rustpython_common::lock::PyRwLock; use std::fmt; pub type SetContentType = dictdatatype::Dict<()>; @@ -842,11 +842,6 @@ impl PySetIterator { self.internal .read() .length_hint(|_| Some(self.size.entries_size), vm) - // if let IterStatus::Exhausted = self.status.load() { - // 0 - // } else { - // self.dict.len_from_entry_index(self.position.load()) - // } } #[pymethod(magic)] @@ -867,41 +862,25 @@ impl PySetIterator { impl IteratorIterable for PySetIterator {} impl SlotIterator for PySetIterator { fn next(zelf: &PyRef, vm: &VirtualMachine) -> PyResult { - let mut status = PyRwLockWriteGuard::map(zelf.internal.write(), |x| &mut x.status); - let mut position = PyRwLockWriteGuard::map(zelf.internal.write(), |x| &mut x.position); - if let IterStatus::Active(dict) = &*status { + let mut internal = zelf.internal.write(); + if let IterStatus::Active(dict) = &internal.status { if dict.has_changed_size(&zelf.size) { - *status = IterStatus::Exhausted; + internal.status = IterStatus::Exhausted; return Err(vm.new_runtime_error("set changed size during iteration".to_owned())); } - match dict.next_entry(&mut *position) { - Some((key, _)) => Ok(key), + match dict.next_entry(internal.position) { + Some((position, key, _)) => { + internal.position = position; + Ok(key) + } None => { - *status = IterStatus::Exhausted; + internal.status = IterStatus::Exhausted; Err(vm.new_stop_iteration()) } } } else { Err(vm.new_stop_iteration()) } - // match zelf.status.load() { - // IterStatus::Exhausted => Err(vm.new_stop_iteration()), - // IterStatus::Active => { - // if zelf.dict.has_changed_size(&zelf.size) { - // zelf.status.store(IterStatus::Exhausted); - // return Err( - // vm.new_runtime_error("set changed size during iteration".to_owned()) - // ); - // } - // match zelf.dict.next_entry_atomic(&zelf.position) { - // Some((key, _)) => Ok(key), - // None => { - // zelf.status.store(IterStatus::Exhausted); - // Err(vm.new_stop_iteration()) - // } - // } - // } - // } } } diff --git a/vm/src/dictdatatype.rs b/vm/src/dictdatatype.rs index c542eb439d..cfc6426b3c 100644 --- a/vm/src/dictdatatype.rs +++ b/vm/src/dictdatatype.rs @@ -478,24 +478,24 @@ impl Dict { self.read().size() } - pub fn next_entry(&self, position: &mut EntryIndex) -> Option<(PyObjectRef, T)> { + pub fn next_entry(&self, mut position: EntryIndex) -> Option<(usize, PyObjectRef, T)> { let inner = self.read(); loop { - let entry = inner.entries.get(*position)?; - *position += 1; + let entry = inner.entries.get(position)?; + position += 1; if let Some(entry) = entry { - break Some((entry.key.clone(), entry.value.clone())); + break Some((position, entry.key.clone(), entry.value.clone())); } } } - pub fn prev_entry(&self, position: &mut EntryIndex) -> Option<(PyObjectRef, T)> { + pub fn prev_entry(&self, mut position: EntryIndex) -> Option<(usize, PyObjectRef, T)> { let inner = self.read(); loop { - let entry = inner.entries.get(*position)?; - *position = position.checked_sub(1)?; + let entry = inner.entries.get(position)?; + position = position.saturating_sub(1); if let Some(entry) = entry { - break Some((entry.key.clone(), entry.value.clone())); + break Some((position, entry.key.clone(), entry.value.clone())); } } }