From 8042ff8a86f6c0a966f77b7fbfcca66eb54ab6cb Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 15 May 2020 16:19:00 +0300 Subject: [PATCH 1/6] Make PySocket ThreadSafe --- vm/src/stdlib/socket.rs | 58 +++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/vm/src/stdlib/socket.rs b/vm/src/stdlib/socket.rs index 2969ae5a7a..7957a4d8fa 100644 --- a/vm/src/stdlib/socket.rs +++ b/vm/src/stdlib/socket.rs @@ -1,9 +1,10 @@ -use std::cell::{Cell, Ref, RefCell}; use std::io::{self, prelude::*}; use std::net::{Ipv4Addr, Shutdown, SocketAddr, ToSocketAddrs}; +use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use std::time::Duration; use byteorder::{BigEndian, ByteOrder}; +use crossbeam_utils::atomic::AtomicCell; use gethostname::gethostname; #[cfg(all(unix, not(target_os = "redox")))] use nix::unistd::sethostname; @@ -21,7 +22,8 @@ use crate::obj::objstr::{PyString, PyStringRef}; use crate::obj::objtuple::PyTupleRef; use crate::obj::objtype::PyClassRef; use crate::pyobject::{ - Either, IntoPyObject, PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, + Either, IntoPyObject, PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue, ThreadSafe, + TryFromObject, }; use crate::vm::VirtualMachine; @@ -44,12 +46,14 @@ mod c { #[pyclass] #[derive(Debug)] pub struct PySocket { - kind: Cell, - family: Cell, - proto: Cell, - sock: RefCell, + kind: AtomicCell, + family: AtomicCell, + proto: AtomicCell, + sock: RwLock, } +impl ThreadSafe for PySocket {} + impl PyValue for PySocket { fn class(vm: &VirtualMachine) -> PyClassRef { vm.class("_socket", "socket") @@ -60,17 +64,21 @@ pub type PySocketRef = PyRef; #[pyimpl(flags(BASETYPE))] impl PySocket { - fn sock(&self) -> Ref { - self.sock.borrow() + fn sock(&self) -> RwLockReadGuard<'_, Socket> { + self.sock.read().unwrap() + } + + fn sock_mut(&self) -> RwLockWriteGuard<'_, Socket> { + self.sock.write().unwrap() } #[pyslot] fn tp_new(cls: PyClassRef, _args: PyFuncArgs, vm: &VirtualMachine) -> PyResult> { PySocket { - kind: Cell::default(), - family: Cell::default(), - proto: Cell::default(), - sock: RefCell::new(invalid_sock()), + kind: AtomicCell::default(), + family: AtomicCell::default(), + proto: AtomicCell::default(), + sock: RwLock::new(invalid_sock()), } .into_ref_with_type(vm, cls) } @@ -103,12 +111,12 @@ impl PySocket { ) .map_err(|err| convert_sock_error(vm, err))?; - self.family.set(family); - self.kind.set(socket_kind); - self.proto.set(proto); + self.family.store(family); + self.kind.store(socket_kind); + self.proto.store(proto); sock }; - self.sock.replace(sock); + *self.sock.write().unwrap() = sock; Ok(()) } @@ -191,7 +199,7 @@ impl PySocket { #[pymethod] fn sendall(&self, bytes: PyBytesLike, vm: &VirtualMachine) -> PyResult<()> { bytes - .with_ref(|b| self.sock.borrow_mut().write_all(b)) + .with_ref(|b| self.sock_mut().write_all(b)) .map_err(|err| convert_sock_error(vm, err)) } @@ -206,11 +214,11 @@ impl PySocket { #[pymethod] fn close(&self) { - self.sock.replace(invalid_sock()); + *self.sock_mut() = invalid_sock(); } #[pymethod] fn detach(&self) -> RawSocket { - into_sock_fileno(self.sock.replace(invalid_sock())) + into_sock_fileno(std::mem::replace(&mut *self.sock_mut(), invalid_sock())) } #[pymethod] @@ -384,29 +392,29 @@ impl PySocket { #[pyproperty(name = "type")] fn kind(&self) -> i32 { - self.kind.get() + self.kind.load() } #[pyproperty] fn family(&self) -> i32 { - self.family.get() + self.family.load() } #[pyproperty] fn proto(&self) -> i32 { - self.proto.get() + self.proto.load() } } impl io::Read for PySocketRef { fn read(&mut self, buf: &mut [u8]) -> io::Result { - ::read(&mut self.sock.borrow_mut(), buf) + ::read(&mut self.sock_mut(), buf) } } impl io::Write for PySocketRef { fn write(&mut self, buf: &[u8]) -> io::Result { - ::write(&mut self.sock.borrow_mut(), buf) + ::write(&mut self.sock_mut(), buf) } fn flush(&mut self) -> io::Result<()> { - ::flush(&mut self.sock.borrow_mut()) + ::flush(&mut self.sock_mut()) } } From 13f2377600ce80e9bebca3ed79bb878ea634ffaa Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 15 May 2020 16:29:20 +0300 Subject: [PATCH 2/6] Make Popen ThreadSafe --- vm/src/stdlib/subprocess.rs | 45 +++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/vm/src/stdlib/subprocess.rs b/vm/src/stdlib/subprocess.rs index 9694edb82a..2848893b94 100644 --- a/vm/src/stdlib/subprocess.rs +++ b/vm/src/stdlib/subprocess.rs @@ -1,7 +1,7 @@ -use std::cell::RefCell; use std::ffi::OsString; use std::fs::File; use std::io::ErrorKind; +use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use std::time::Duration; use crate::function::OptionalArg; @@ -9,17 +9,19 @@ use crate::obj::objbytes::PyBytesRef; use crate::obj::objlist::PyListRef; use crate::obj::objstr::{self, PyStringRef}; use crate::obj::objtype::PyClassRef; -use crate::pyobject::{Either, IntoPyObject, PyObjectRef, PyRef, PyResult, PyValue}; +use crate::pyobject::{Either, IntoPyObject, PyObjectRef, PyRef, PyResult, PyValue, ThreadSafe}; use crate::stdlib::io::io_open; use crate::stdlib::os::{convert_io_error, raw_file_number, rust_file}; use crate::vm::VirtualMachine; #[derive(Debug)] struct Popen { - process: RefCell, + process: RwLock, args: PyObjectRef, } +impl ThreadSafe for Popen {} + impl PyValue for Popen { fn class(vm: &VirtualMachine) -> PyClassRef { vm.class("_subprocess", "Popen") @@ -103,6 +105,14 @@ fn convert_to_file_io(file: &Option, mode: &str, vm: &VirtualMachine) -> P } impl PopenRef { + fn borrow_process(&self) -> RwLockReadGuard<'_, subprocess::Popen> { + self.process.read().unwrap() + } + + fn borrow_process_mut(&self) -> RwLockWriteGuard<'_, subprocess::Popen> { + self.process.write().unwrap() + } + fn new(cls: PyClassRef, args: PopenArgs, vm: &VirtualMachine) -> PyResult { let stdin = convert_redirection(args.stdin, vm)?; let stdout = convert_redirection(args.stdout, vm)?; @@ -130,27 +140,26 @@ impl PopenRef { .map_err(|s| vm.new_os_error(format!("Could not start program: {}", s)))?; Popen { - process: RefCell::new(process), + process: RwLock::new(process), args: args.args.into_object(), } .into_ref_with_type(vm, cls) } fn poll(self) -> Option { - self.process.borrow_mut().poll() + self.borrow_process_mut().poll() } fn return_code(self) -> Option { - self.process.borrow().exit_status() + self.borrow_process().exit_status() } fn wait(self, args: PopenWaitArgs, vm: &VirtualMachine) -> PyResult { let timeout = match args.timeout { Some(timeout) => self - .process - .borrow_mut() + .borrow_process_mut() .wait_timeout(Duration::new(timeout, 0)), - None => self.process.borrow_mut().wait().map(Some), + None => self.borrow_process_mut().wait().map(Some), } .map_err(|s| vm.new_os_error(format!("Could not start program: {}", s)))?; if let Some(exit) = timeout { @@ -167,27 +176,25 @@ impl PopenRef { } fn stdin(self, vm: &VirtualMachine) -> PyResult { - convert_to_file_io(&self.process.borrow().stdin, "wb", vm) + convert_to_file_io(&self.borrow_process().stdin, "wb", vm) } fn stdout(self, vm: &VirtualMachine) -> PyResult { - convert_to_file_io(&self.process.borrow().stdout, "rb", vm) + convert_to_file_io(&self.borrow_process().stdout, "rb", vm) } fn stderr(self, vm: &VirtualMachine) -> PyResult { - convert_to_file_io(&self.process.borrow().stderr, "rb", vm) + convert_to_file_io(&self.borrow_process().stderr, "rb", vm) } fn terminate(self, vm: &VirtualMachine) -> PyResult<()> { - self.process - .borrow_mut() + self.borrow_process_mut() .terminate() .map_err(|err| convert_io_error(vm, err)) } fn kill(self, vm: &VirtualMachine) -> PyResult<()> { - self.process - .borrow_mut() + self.borrow_process_mut() .kill() .map_err(|err| convert_io_error(vm, err)) } @@ -202,7 +209,7 @@ impl PopenRef { OptionalArg::Present(ref bytes) => Some(bytes.get_value().to_vec()), OptionalArg::Missing => None, }; - let mut communicator = self.process.borrow_mut().communicate_start(bytes); + let mut communicator = self.borrow_process_mut().communicate_start(bytes); if let OptionalArg::Present(timeout) = args.timeout { communicator = communicator.limit_time(Duration::new(timeout, 0)); } @@ -217,7 +224,7 @@ impl PopenRef { } fn pid(self) -> Option { - self.process.borrow().pid() + self.borrow_process().pid() } fn enter(self) -> Self { @@ -230,7 +237,7 @@ impl PopenRef { _exception_value: PyObjectRef, _traceback: PyObjectRef, ) { - let mut process = self.process.borrow_mut(); + let mut process = self.borrow_process_mut(); process.stdout.take(); process.stdin.take(); process.stderr.take(); From 676c6413664a271720e9a018a15da54b6782ebe9 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 15 May 2020 16:32:24 +0300 Subject: [PATCH 3/6] Mark PySymbol, PySymbolTable as ThreadSafe --- vm/src/stdlib/symtable.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vm/src/stdlib/symtable.rs b/vm/src/stdlib/symtable.rs index f95c9fc916..440ad69498 100644 --- a/vm/src/stdlib/symtable.rs +++ b/vm/src/stdlib/symtable.rs @@ -5,7 +5,7 @@ use rustpython_parser::parser; use crate::obj::objstr::PyStringRef; use crate::obj::objtype::PyClassRef; -use crate::pyobject::{PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue}; +use crate::pyobject::{PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue, ThreadSafe}; use crate::vm::VirtualMachine; pub fn make_module(vm: &VirtualMachine) -> PyObjectRef { @@ -72,6 +72,8 @@ struct PySymbolTable { symtable: symboltable::SymbolTable, } +impl ThreadSafe for PySymbolTable {} + impl fmt::Debug for PySymbolTable { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "SymbolTable()") @@ -158,6 +160,8 @@ struct PySymbol { symbol: symboltable::Symbol, } +impl ThreadSafe for PySymbol {} + impl fmt::Debug for PySymbol { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "Symbol()") From 3650a2cdce2f3005a4fdf9af8e3cea895feb3e95 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 15 May 2020 16:34:54 +0300 Subject: [PATCH 4/6] Make PyUCD as ThreadSafe --- vm/src/stdlib/unicodedata.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/vm/src/stdlib/unicodedata.rs b/vm/src/stdlib/unicodedata.rs index 03c23064dc..c7342d0ef1 100644 --- a/vm/src/stdlib/unicodedata.rs +++ b/vm/src/stdlib/unicodedata.rs @@ -5,7 +5,7 @@ use crate::function::OptionalArg; use crate::obj::objstr::PyStringRef; use crate::obj::objtype::PyClassRef; -use crate::pyobject::{PyClassImpl, PyObject, PyObjectRef, PyResult, PyValue}; +use crate::pyobject::{PyClassImpl, PyObject, PyObjectRef, PyResult, PyValue, ThreadSafe}; use crate::vm::VirtualMachine; use itertools::Itertools; @@ -60,6 +60,8 @@ struct PyUCD { unic_version: UnicodeVersion, } +impl ThreadSafe for PyUCD {} + impl PyValue for PyUCD { fn class(vm: &VirtualMachine) -> PyClassRef { vm.class("unicodedata", "UCD") From abdea909d3d59be233b2928e2cf2c9ca8628eb2e Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 15 May 2020 16:40:35 +0300 Subject: [PATCH 5/6] Make PyHKEY ThreadSafe --- vm/src/stdlib/winreg.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/vm/src/stdlib/winreg.rs b/vm/src/stdlib/winreg.rs index e08eabc8cc..018a5eadeb 100644 --- a/vm/src/stdlib/winreg.rs +++ b/vm/src/stdlib/winreg.rs @@ -1,14 +1,15 @@ #![allow(non_snake_case)] - -use std::cell::{Ref, RefCell}; use std::convert::TryInto; use std::io; +use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use super::os; use crate::function::OptionalArg; use crate::obj::objstr::PyStringRef; use crate::obj::objtype::PyClassRef; -use crate::pyobject::{PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject}; +use crate::pyobject::{ + PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue, ThreadSafe, TryFromObject, +}; use crate::VirtualMachine; use winapi::shared::winerror; @@ -17,10 +18,12 @@ use winreg::{enums::RegType, RegKey, RegValue}; #[pyclass] #[derive(Debug)] struct PyHKEY { - key: RefCell, + key: RwLock, } type PyHKEYRef = PyRef; +impl ThreadSafe for PyHKEY {} + impl PyValue for PyHKEY { fn class(vm: &VirtualMachine) -> PyClassRef { vm.class("winreg", "HKEYType") @@ -31,24 +34,28 @@ impl PyValue for PyHKEY { impl PyHKEY { fn new(key: RegKey) -> Self { Self { - key: RefCell::new(key), + key: RwLock::new(key), } } - fn key(&self) -> Ref { - self.key.borrow() + fn key(&self) -> RwLockReadGuard<'_, RegKey> { + self.key.read().unwrap() + } + + fn key_mut(&self) -> RwLockWriteGuard<'_, RegKey> { + self.key.write().unwrap() } #[pymethod] fn Close(&self) { let null_key = RegKey::predef(0 as winreg::HKEY); - let key = self.key.replace(null_key); + let key = std::mem::replace(&mut *self.key_mut(), null_key); drop(key); } #[pymethod] fn Detach(&self) -> usize { let null_key = RegKey::predef(0 as winreg::HKEY); - let key = self.key.replace(null_key); + let key = std::mem::replace(&mut *self.key_mut(), null_key); let handle = key.raw_handle(); std::mem::forget(key); handle as usize From fc21d727042a3a01651108e23f56cdf943322149 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 15 May 2020 17:42:32 +0300 Subject: [PATCH 6/6] Add unsafe Sync for PyHKEY and Popen --- vm/src/stdlib/subprocess.rs | 4 ++++ vm/src/stdlib/winreg.rs | 3 +++ 2 files changed, 7 insertions(+) diff --git a/vm/src/stdlib/subprocess.rs b/vm/src/stdlib/subprocess.rs index 2848893b94..986ddbcef1 100644 --- a/vm/src/stdlib/subprocess.rs +++ b/vm/src/stdlib/subprocess.rs @@ -20,6 +20,10 @@ struct Popen { args: PyObjectRef, } +// Remove once https://github.com/hniksic/rust-subprocess/issues/42 is resolved +#[cfg(windows)] +unsafe impl Sync for Popen {} + impl ThreadSafe for Popen {} impl PyValue for Popen { diff --git a/vm/src/stdlib/winreg.rs b/vm/src/stdlib/winreg.rs index 018a5eadeb..08ef1092d2 100644 --- a/vm/src/stdlib/winreg.rs +++ b/vm/src/stdlib/winreg.rs @@ -22,6 +22,9 @@ struct PyHKEY { } type PyHKEYRef = PyRef; +// TODO: fix this +unsafe impl Sync for PyHKEY {} + impl ThreadSafe for PyHKEY {} impl PyValue for PyHKEY {