Merge pull request #2288 from qingshi163/dev5

fix buffer resizable non-thread safe

Lib/test/test_bytes.py

@@ -1541,8 +1541,6 @@ class ByteArrayTest(BaseBytesTest, unittest.TestCase):
         self.assertEqual(b, b"")
         self.assertEqual(c, b"")
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_resize_forbidden(self):
         # #4509: can't resize a bytearray when there are buffer exports, even
         # if it wouldn't reallocate the underlying buffer.

vm/src/builtins/bytearray.rs

@@ -1,7 +1,7 @@
 //! Implementation of the python bytearray object.
 use super::bytes::{PyBytes, PyBytesRef};
 use super::int::PyIntRef;
-use super::memory::{Buffer, BufferOptions};
+use super::memory::{Buffer, BufferOptions, ResizeGuard};
 use super::pystr::PyStrRef;
 use super::pytype::PyTypeRef;
 use super::tuple::PyTupleRef;
@@ -141,8 +141,7 @@ impl PyByteArray {
 
     #[pymethod(name = "__iadd__")]
     fn iadd(zelf: PyRef<Self>, other: PyBytesLike, vm: &VirtualMachine) -> PyResult<PyRef<Self>> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().iadd(&*other.borrow_value());
+        zelf.try_resizable(vm)?.iadd(&*other.borrow_value());
         Ok(zelf)
     }
 
@@ -169,21 +168,26 @@ impl PyByteArray {
     ) -> PyResult<()> {
         match needle {
             SequenceIndex::Int(int) => zelf.borrow_value_mut().setindex(int, value, vm),
-            SequenceIndex::Slice(slice) => match (zelf.is(&value), zelf.is_resizable()) {
-                (true, true) => zelf.borrow_value_mut().setslice_from_self(slice, vm),
-                (true, false) => zelf
-                    .borrow_value_mut()
-                    .setslice_from_self_no_resize(slice, vm),
-                (false, true) => zelf.borrow_value_mut().setslice(slice, value, vm),
-                (false, false) => zelf.borrow_value_mut().setslice_no_resize(slice, value, vm),
-            },
+            SequenceIndex::Slice(slice) => {
+                if let Ok(mut w) = zelf.try_resizable(vm) {
+                    if zelf.is(&value) {
+                        w.setslice_from_self(slice, vm)
+                    } else {
+                        w.setslice(slice, value, vm)
+                    }
+                } else if zelf.is(&value) {
+                    zelf.borrow_value_mut()
+                        .setslice_from_self_no_resize(slice, vm)
+                } else {
+                    zelf.borrow_value_mut().setslice_no_resize(slice, value, vm)
+                }
+            }
         }
     }
 
     #[pymethod(name = "__delitem__")]
     fn delitem(zelf: PyRef<Self>, needle: SequenceIndex, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().delitem(needle, vm)
+        zelf.try_resizable(vm)?.delitem(needle, vm)
     }
 
     #[pymethod(name = "isalnum")]
@@ -350,7 +354,7 @@ impl PyByteArray {
 
     #[pymethod(name = "remove")]
     fn remove(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        self.borrow_value_mut().remove(value, vm)
+        self.try_resizable(vm)?.remove(value, vm)
     }
 
     #[pymethod(name = "translate")]
@@ -470,8 +474,7 @@ impl PyByteArray {
 
     #[pymethod(name = "clear")]
     fn clear(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().elements.clear();
+        zelf.try_resizable(vm)?.elements.clear();
         Ok(())
     }
 
@@ -482,18 +485,16 @@ impl PyByteArray {
 
     #[pymethod(name = "append")]
     fn append(zelf: PyRef<Self>, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().append(value, vm)
+        zelf.try_resizable(vm)?.append(value, vm)
     }
 
     #[pymethod(name = "extend")]
     fn extend(zelf: PyRef<Self>, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
         if zelf.is(&value) {
-            zelf.borrow_value_mut().irepeat(2);
+            zelf.try_resizable(vm)?.irepeat(2);
             Ok(())
         } else {
-            zelf.borrow_value_mut().extend(value, vm)
+            zelf.try_resizable(vm)?.extend(value, vm)
         }
     }
 
@@ -504,15 +505,13 @@ impl PyByteArray {
         value: PyObjectRef,
         vm: &VirtualMachine,
     ) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().insert(index, value, vm)
+        zelf.try_resizable(vm)?.insert(index, value, vm)
     }
 
     #[pymethod(name = "pop")]
     fn pop(zelf: PyRef<Self>, index: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<u8> {
-        zelf.try_resizable(vm)?;
         let index = index.unwrap_or(-1);
-        zelf.borrow_value_mut().pop(index, vm)
+        zelf.try_resizable(vm)?.pop(index, vm)
     }
 
     #[pymethod(name = "title")]
@@ -528,8 +527,7 @@ impl PyByteArray {
 
     #[pymethod(name = "__imul__")]
     fn imul(zelf: PyRef<Self>, n: isize, vm: &VirtualMachine) -> PyResult<PyRef<Self>> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().irepeat(n);
+        zelf.try_resizable(vm)?.irepeat(n);
         Ok(zelf)
     }
 
@@ -607,10 +605,6 @@ impl Buffer for PyByteArrayRef {
         }
     }
 
-    fn is_resizable(&self) -> bool {
-        self.exports.load() == 0
-    }
-
     fn get_options(&self) -> BorrowedValue<BufferOptions> {
         let guard = self.buffer_options.upgradable_read();
         let guard = if guard.is_none() {
@@ -628,6 +622,20 @@ impl Buffer for PyByteArrayRef {
     }
 }
 
+impl<'a> ResizeGuard<'a> for PyByteArray {
+    type Resizable = PyRwLockWriteGuard<'a, PyBytesInner>;
+
+    fn try_resizable(&'a self, vm: &VirtualMachine) -> PyResult<Self::Resizable> {
+        let w = self.borrow_value_mut();
+        if self.exports.load() == 0 {
+            Ok(w)
+        } else {
+            Err(vm
+                .new_buffer_error("Existing exports of data: object cannot be re-sized".to_owned()))
+        }
+    }
+}
+
 impl Unhashable for PyByteArray {}
 
 // fn set_value(obj: &PyObjectRef, value: Vec<u8>) {

vm/src/builtins/bytes.rs

@@ -493,10 +493,6 @@ impl Buffer for PyBytesRef {
         unreachable!("bytes is not mutable")
     }
 
-    fn is_resizable(&self) -> bool {
-        false
-    }
-
     fn release(&self) {}
 
     fn get_options(&self) -> BorrowedValue<BufferOptions> {

vm/src/builtins/memory.rs

@@ -47,7 +47,6 @@ pub trait Buffer: Debug + PyThreadingConstraint {
     fn obj_bytes(&self) -> BorrowedValue<[u8]>;
     fn obj_bytes_mut(&self) -> BorrowedValueMut<[u8]>;
     fn release(&self);
-    fn is_resizable(&self) -> bool;
 
     fn as_contiguous(&self) -> Option<BorrowedValue<[u8]>> {
         let options = self.get_options();
@@ -68,17 +67,6 @@ pub trait Buffer: Debug + PyThreadingConstraint {
     fn to_contiguous(&self) -> Vec<u8> {
         self.obj_bytes().to_vec()
     }
-
-    fn try_resizable(&self, vm: &VirtualMachine) -> PyResult<()> {
-        if self.is_resizable() {
-            Ok(())
-        } else {
-            Err(vm.new_exception_msg(
-                vm.ctx.exceptions.buffer_error.clone(),
-                "Existing exports of data: object cannot be re-sized".to_owned(),
-            ))
-        }
-    }
 }
 
 #[derive(Debug, Clone)]
@@ -94,6 +82,11 @@ pub struct BufferOptions {
     pub strides: Vec<isize>,
 }
 
+pub(crate) trait ResizeGuard<'a> {
+    type Resizable: 'a;
+    fn try_resizable(&'a self, vm: &VirtualMachine) -> PyResult<Self::Resizable>;
+}
+
 impl Default for BufferOptions {
     fn default() -> Self {
         BufferOptions {
@@ -709,11 +702,6 @@ impl Buffer for PyMemoryViewRef {
         }
     }
 
-    fn is_resizable(&self) -> bool {
-        // memoryview cannot resize
-        false
-    }
-
     fn as_contiguous(&self) -> Option<BorrowedValue<[u8]>> {
         let options = self.get_options();
         if !options.contiguous {

vm/src/sliceable.rs

@@ -32,8 +32,7 @@ pub trait PySliceableSequenceMut {
                 self.do_set_range(range, items);
                 Ok(())
             } else {
-                Err(vm.new_exception_msg(
-                    vm.ctx.exceptions.buffer_error.clone(),
+                Err(vm.new_buffer_error(
                     "Existing exports of data: object cannot be re-sized".to_owned(),
                 ))
             };
@@ -54,11 +53,9 @@ pub trait PySliceableSequenceMut {
                 self.do_replace_indexes(indexes, items);
                 Ok(())
             } else {
-                Err(vm.new_value_error(format!(
-                    "attempt to assign sequence of size {} to extended slice of size {}",
-                    items.len(),
-                    slicelen
-                )))
+                Err(vm.new_buffer_error(
+                    "Existing exports of data: object cannot be re-sized".to_owned(),
+                ))
             }
         } else {
             // edge case, step is too big for usize
@@ -74,11 +71,9 @@ pub trait PySliceableSequenceMut {
             } {
                 Ok(())
             } else {
-                Err(vm.new_value_error(format!(
-                    "attempt to assign sequence of size {} to extended slice of size {}",
-                    items.len(),
-                    slicelen
-                )))
+                Err(vm.new_buffer_error(
+                    "Existing exports of data: object cannot be re-sized".to_owned(),
+                ))
             }
         }
     }

vm/src/stdlib/array.rs

@@ -1,7 +1,7 @@
 use crate::builtins::bytes::PyBytesRef;
 use crate::builtins::float::try_float;
 use crate::builtins::list::PyList;
-use crate::builtins::memory::{Buffer, BufferOptions};
+use crate::builtins::memory::{Buffer, BufferOptions, ResizeGuard};
 use crate::builtins::pystr::PyStrRef;
 use crate::builtins::pytype::PyTypeRef;
 use crate::builtins::slice::PySliceRef;
@@ -39,7 +39,7 @@ impl fmt::Display for ArrayTypeSpecifierError {
 macro_rules! def_array_enum {
     ($(($n:ident, $t:ident, $c:literal)),*$(,)?) => {
         #[derive(Debug, Clone)]
-        enum ArrayContentType {
+        pub(crate) enum ArrayContentType {
             $($n(Vec<$t>),)*
         }
 
@@ -508,13 +508,15 @@ impl PyArray {
         let spec = spec.borrow_value().chars().exactly_one().map_err(|_| {
             vm.new_type_error("array() argument 1 must be a unicode character, not str".to_owned())
         })?;
-        let array =
+        let mut array =
             ArrayContentType::from_char(spec).map_err(|err| vm.new_value_error(err.to_string()))?;
-        let zelf: PyArray = array.into();
+        // TODO: support buffer protocol
         if let OptionalArg::Present(init) = init {
-            zelf.extend_from_iterable(init, vm)?;
+            for obj in init.iter(vm)? {
+                array.push(obj?, vm)?;
+            }
         }
-        zelf.into_ref_with_type(vm, cls)
+        Self::from(array).into_ref_with_type(vm, cls)
     }
 
     #[pyproperty]
@@ -529,8 +531,7 @@ impl PyArray {
 
     #[pymethod]
     fn append(zelf: PyRef<Self>, x: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().push(x, vm)
+        zelf.try_resizable(vm)?.push(x, vm)
     }
 
     #[pymethod]
@@ -546,42 +547,36 @@ impl PyArray {
 
     #[pymethod]
     fn remove(zelf: PyRef<Self>, x: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().remove(x, vm)
-    }
-
-    fn extend_from_iterable(&self, iter: PyIterable, vm: &VirtualMachine) -> PyResult<()> {
-        let mut array = self.borrow_value_mut();
-        for obj in iter.iter(vm)? {
-            array.push(obj?, vm)?;
-        }
-        Ok(())
+        zelf.try_resizable(vm)?.remove(x, vm)
     }
 
     #[pymethod]
     fn extend(zelf: PyRef<Self>, obj: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
+        let mut w = zelf.try_resizable(vm)?;
         if zelf.is(&obj) {
-            zelf.borrow_value_mut().imul(2);
+            w.imul(2);
             Ok(())
         } else if let Some(array) = obj.payload::<PyArray>() {
-            zelf.borrow_value_mut().iadd(&*array.borrow_value(), vm)
+            w.iadd(&*array.borrow_value(), vm)
         } else {
             let iter = PyIterable::try_from_object(vm, obj)?;
-            zelf.extend_from_iterable(iter, vm)
+            // zelf.extend_from_iterable(iter, vm)
+            for obj in iter.iter(vm)? {
+                w.push(obj?, vm)?;
+            }
+            Ok(())
         }
     }
 
     #[pymethod]
     fn frombytes(zelf: PyRef<Self>, b: PyBytesRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
         let b = b.borrow_value();
         let itemsize = zelf.borrow_value().itemsize();
         if b.len() % itemsize != 0 {
             return Err(vm.new_value_error("bytes length not a multiple of item size".to_owned()));
         }
         if b.len() / itemsize > 0 {
-            zelf.borrow_value_mut().frombytes(&b);
+            zelf.try_resizable(vm)?.frombytes(&b);
         }
         Ok(())
     }
@@ -598,19 +593,19 @@ impl PyArray {
 
     #[pymethod]
     fn insert(zelf: PyRef<Self>, i: isize, x: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
-        let i = saturate_index(i, zelf.len());
-        zelf.borrow_value_mut().insert(i, x, vm)
+        let mut w = zelf.try_resizable(vm)?;
+        let i = saturate_index(i, w.len());
+        w.insert(i, x, vm)
     }
 
     #[pymethod]
     fn pop(zelf: PyRef<Self>, i: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult {
-        zelf.try_resizable(vm)?;
-        if zelf.len() == 0 {
+        let mut w = zelf.try_resizable(vm)?;
+        if w.len() == 0 {
             Err(vm.new_index_error("pop from empty array".to_owned()))
         } else {
-            let i = zelf.borrow_value().idx(i.unwrap_or(-1), "pop", vm)?;
-            Ok(zelf.borrow_value_mut().pop(i, vm))
+            let i = w.idx(i.unwrap_or(-1), "pop", vm)?;
+            Ok(w.pop(i, vm))
         }
     }
 
@@ -639,9 +634,8 @@ impl PyArray {
 
     #[pymethod]
     fn fromlist(zelf: PyRef<Self>, obj: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
         if let Some(list) = obj.payload::<PyList>() {
-            zelf.borrow_value_mut().fromlist(list, vm)
+            zelf.try_resizable(vm)?.fromlist(list, vm)
         } else {
             Err(vm.new_type_error("arg must be list".to_owned()))
         }
@@ -674,7 +668,6 @@ impl PyArray {
         obj: PyObjectRef,
         vm: &VirtualMachine,
     ) -> PyResult<()> {
-        // FIXME check resizable
         match needle {
             Either::A(i) => zelf.borrow_value_mut().setitem_by_idx(i, obj, vm),
             Either::B(slice) => {
@@ -697,8 +690,8 @@ impl PyArray {
                         }
                     }
                 };
-                if zelf.is_resizable() {
-                    zelf.borrow_value_mut().setitem_by_slice(slice, items, vm)
+                if let Ok(mut w) = zelf.try_resizable(vm) {
+                    w.setitem_by_slice(slice, items, vm)
                 } else {
                     zelf.borrow_value_mut()
                         .setitem_by_slice_no_resize(slice, items, vm)
@@ -713,10 +706,9 @@ impl PyArray {
         needle: Either<isize, PySliceRef>,
         vm: &VirtualMachine,
     ) -> PyResult<()> {
-        zelf.try_resizable(vm)?;
         match needle {
-            Either::A(i) => zelf.borrow_value_mut().delitem_by_idx(i, vm),
-            Either::B(slice) => zelf.borrow_value_mut().delitem_by_slice(slice, vm),
+            Either::A(i) => zelf.try_resizable(vm)?.delitem_by_idx(i, vm),
+            Either::B(slice) => zelf.try_resizable(vm)?.delitem_by_slice(slice, vm),
         }
     }
 
@@ -736,12 +728,11 @@ impl PyArray {
 
     #[pymethod(name = "__iadd__")]
     fn iadd(zelf: PyRef<Self>, other: PyObjectRef, vm: &VirtualMachine) -> PyResult<PyRef<Self>> {
-        zelf.try_resizable(vm)?;
         if zelf.is(&other) {
-            zelf.borrow_value_mut().imul(2);
+            zelf.try_resizable(vm)?.imul(2);
             Ok(zelf)
         } else if let Some(other) = other.payload::<PyArray>() {
-            let result = zelf.borrow_value_mut().iadd(&*other.borrow_value(), vm);
+            let result = zelf.try_resizable(vm)?.iadd(&*other.borrow_value(), vm);
             result.map(|_| zelf)
         } else {
             Err(vm.new_type_error(format!(
@@ -763,8 +754,7 @@ impl PyArray {
 
     #[pymethod(name = "__imul__")]
     fn imul(zelf: PyRef<Self>, counter: isize, vm: &VirtualMachine) -> PyResult<PyRef<Self>> {
-        zelf.try_resizable(vm)?;
-        zelf.borrow_value_mut().imul(counter);
+        zelf.try_resizable(vm)?.imul(counter);
         Ok(zelf)
     }
 
@@ -882,10 +872,6 @@ impl Buffer for PyArrayRef {
         }
     }
 
-    fn is_resizable(&self) -> bool {
-        self.exports.load() == 0
-    }
-
     fn get_options(&self) -> BorrowedValue<BufferOptions> {
         let guard = self.buffer_options.upgradable_read();
         let guard = if guard.is_none() {
@@ -906,6 +892,20 @@ impl Buffer for PyArrayRef {
     }
 }
 
+impl<'a> ResizeGuard<'a> for PyArray {
+    type Resizable = PyRwLockWriteGuard<'a, ArrayContentType>;
+
+    fn try_resizable(&'a self, vm: &VirtualMachine) -> PyResult<Self::Resizable> {
+        let w = self.borrow_value_mut();
+        if self.exports.load() == 0 {
+            Ok(w)
+        } else {
+            Err(vm
+                .new_buffer_error("Existing exports of data: object cannot be re-sized".to_owned()))
+        }
+    }
+}
+
 #[pyclass(module = "array", name = "array_iterator")]
 #[derive(Debug)]
 pub struct PyArrayIter {

vm/src/stdlib/io.rs

@@ -20,7 +20,7 @@ mod _io {
     use crate::builtins::bytearray::PyByteArray;
     use crate::builtins::bytes::PyBytesRef;
     use crate::builtins::int;
-    use crate::builtins::memory::{Buffer, BufferOptions, BufferRef, PyMemoryView};
+    use crate::builtins::memory::{Buffer, BufferOptions, BufferRef, PyMemoryView, ResizeGuard};
     use crate::builtins::pybool;
     use crate::builtins::pystr::{self, PyStr, PyStrRef};
     use crate::builtins::pytype::PyTypeRef;
@@ -986,8 +986,7 @@ mod _io {
     impl BytesIORef {
         #[pymethod]
         fn write(self, data: PyBytesLike, vm: &VirtualMachine) -> PyResult<u64> {
-            self.try_resizable(vm)?;
-            let mut buffer = self.buffer(vm)?;
+            let mut buffer = self.try_resizable(vm)?;
             match data.with_ref(|b| buffer.write(b)) {
                 Some(value) => Ok(value),
                 None => Err(vm.new_type_error("Error Writing Bytes".to_owned())),
@@ -1054,8 +1053,7 @@ mod _io {
 
         #[pymethod]
         fn truncate(self, pos: OptionalSize, vm: &VirtualMachine) -> PyResult<()> {
-            self.try_resizable(vm)?;
-            let mut buffer = self.buffer(vm)?;
+            let mut buffer = self.try_resizable(vm)?;
             buffer.truncate(pos.try_usize(vm)?)?;
             Ok(())
         }
@@ -1067,7 +1065,7 @@ mod _io {
 
         #[pymethod]
         fn close(self, vm: &VirtualMachine) -> PyResult<()> {
-            self.try_resizable(vm)?;
+            let _ = self.try_resizable(vm)?;
             self.closed.store(true);
             Ok(())
         }
@@ -1114,9 +1112,20 @@ mod _io {
                 *w = None;
             }
         }
+    }
 
-        fn is_resizable(&self) -> bool {
-            self.exports.load() == 0
+    impl<'a> ResizeGuard<'a> for BytesIO {
+        type Resizable = PyRwLockWriteGuard<'a, BufferedIO>;
+
+        fn try_resizable(&'a self, vm: &VirtualMachine) -> PyResult<Self::Resizable> {
+            let buffer = self.buffer(vm)?;
+            if self.exports.load() == 0 {
+                Ok(buffer)
+            } else {
+                Err(vm.new_buffer_error(
+                    "Existing exports of data: object cannot be re-sized".to_owned(),
+                ))
+            }
         }
     }