Skip to content
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.

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

Use the parking_lot locking primitives #56410

Closed
wants to merge 40 commits into from
Closed
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
7b0ad27
Add parking_lot as submodule
faern Dec 14, 2018
1aab11d
Exclude parking_lot benchmark crate from root workspace
faern Mar 19, 2019
e313500
Add needed winapi bindings
faern Nov 26, 2018
20ab8d2
Use parking_lot::RawRwLock in panicking module
faern Feb 14, 2019
7cafa6d
Use parking_lot::RawRwLock to implement std::sync::RwLock
faern Feb 14, 2019
7c42fc8
Remove unused sys/sys_common::rwlock
faern Feb 20, 2019
3b23add
Use parking_lot::ReentrantMutex in stdio
faern Feb 19, 2019
f58fc9b
Use parking_lot::Mutex for stdin
faern Feb 20, 2019
81a1e21
Remove unused sys/sys_common ReentrantMutex
faern Feb 20, 2019
5a3dd8a
Base Mutex/Condvar on parking_lot primitives
faern Feb 20, 2019
d354b95
Remove unused sys/sys_common::condvar
faern Feb 20, 2019
8edc128
Remove Mutex init, try_lock, raw_unlock and destroy
faern Feb 27, 2019
4f0f8e1
Remove sys/sys_common::mutex::raw functions
faern Feb 27, 2019
a0b366c
Add sync::RawMutex
faern Mar 3, 2019
8dfad4b
Move io::lazy over to RawMutex
faern Mar 3, 2019
d6fab63
Move std::time over to RawMutex
faern Mar 3, 2019
fc379ee
Move at_exit over to RawMutex
faern Mar 3, 2019
142bda2
Move thread_local over to RawMutex
faern Mar 3, 2019
eaaca60
Move thread over to RawMutex
faern Mar 3, 2019
0d679ef
Move unix::args over to RawMutex
faern Mar 3, 2019
e8e355e
Move unix::os over to RawMutex
faern Mar 3, 2019
55f1ed2
Fix unix fork locking to work with parking_lot
faern Mar 3, 2019
c6c6fb1
Use RawMutex in backtrace
faern Feb 19, 2019
f1a805b
Remove unused sys/sys_common::mutex
faern Mar 3, 2019
0b1a6e9
Lock locks outside unsafe blocks
faern Feb 19, 2019
a562580
Use sys_common::mutex in sys::windows::process
faern Feb 21, 2019
b50a20e
Remove unused sys::c items from Windows
faern Feb 21, 2019
6ad5485
Remove unused sgx waitqueue types
faern Apr 1, 2019
0070390
Convert thread parking to use parking_lot primitives
faern Mar 2, 2019
c294204
Move redox::args over to RawMutex
faern Mar 4, 2019
c301ce0
Make redox::os use RawMutex
faern Mar 4, 2019
dd30a55
Remove wasi locking modules
faern Mar 31, 2019
e0fd309
Make cloudabi::abi module public inside libstd
faern Apr 1, 2019
0048440
Update SGX libunwind FFI interface to use new RwLock
faern Apr 1, 2019
e49bbd1
Make Lazy::get not unsafe any more
faern Apr 17, 2019
f40d609
Base Once on parking_lot::Once
faern Apr 18, 2019
56c2805
Add better documentation for RawMutex
faern Apr 20, 2019
7124cae
Improve Lazy documentation
faern Apr 20, 2019
6695be8
Base park/unpark directly on parking_lot_core
faern May 5, 2019
795285f
Fix park/unpark after feedback
faern May 12, 2019
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
Prev Previous commit
Next Next commit
Fix unix fork locking to work with parking_lot
faern committed May 4, 2019
commit 55f1ed2ae5fd55c46ae119b16f89919b3064694d
18 changes: 9 additions & 9 deletions src/libstd/sys/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::io::{self, Error, ErrorKind};
use crate::mem;
use crate::ptr;
use crate::sys::cvt;
use crate::sys::process::process_common::*;
@@ -35,17 +36,16 @@ impl Command {
// accessing the `environ` pointer ourselves). Make sure no other thread
// is accessing the environment when we do the fork itself.
//
// Note that as soon as we're done with the fork there's no need to hold
// a lock any more because the parent won't do anything and the child is
// in its own process.
let result = unsafe {
let _env_lock = sys::os::env_lock();
cvt(libc::fork())?
};

// Note that as soon as we're done with the fork the parent can stop
// holding the lock, because it won't do anything. And the child must
// forget it, keeping it locked. This is because parking_lot is
// not fork safe and the child process must not lock or unlock any
// locks before it gets to libc::execvp.
let pid = unsafe {
match result {
let _env_lock = sys::os::env_lock();
match cvt(libc::fork())? {
0 => {
mem::forget(_env_lock);
drop(input);
let err = self.do_exec(theirs, envp.as_ref());
let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32;
17 changes: 1 addition & 16 deletions src/test/run-pass/command-pre-exec.rs
Original file line number Diff line number Diff line change
@@ -17,8 +17,7 @@ fn main() {
if let Some(arg) = env::args().nth(1) {
match &arg[..] {
"test1" => println!("hello2"),
"test2" => assert_eq!(env::var("FOO").unwrap(), "BAR"),
"test3" => assert_eq!(env::current_dir().unwrap().to_str().unwrap(), "/"),
"test2" => assert_eq!(env::current_dir().unwrap().to_str().unwrap(), "/"),
"empty" => {}
_ => panic!("unknown argument: {}", arg),
}
@@ -44,20 +43,6 @@ fn main() {
let output = unsafe {
Command::new(&me)
.arg("test2")
.pre_exec(|| {
env::set_var("FOO", "BAR");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this test, since it was testing something one should not be doing in the first place, and with the new lock impl this caused a deadlock.

If the parent process is multi threaded, a child process is not supposed to call anything but syscalls after the fork. Here env::set_var will try to lock the ENV_LOCK mutex which triggers a deadlock because of how parking_lot is not fork-safe. See the comment around the fork call.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a problem... I argued for pre_exec becoming unsafe for a reason, but it is still useful for extra "setup" that might have to be done before the exec in the forked-off process, and it doesn't seem too far-fetched to assume that someone might want to set env vars there. I know Command can do that, but someone might either not be aware or it doesn't really fit the way their code is written.

Ok(())
})
.output()
.unwrap()
};
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert!(output.stdout.is_empty());

let output = unsafe {
Command::new(&me)
.arg("test3")
.pre_exec(|| {
env::set_current_dir("/").unwrap();
Ok(())