Implement reentrant mutexes and make stdio use them #24029
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
I wonder if we should consider re-entrant locking. |
|
Yes I think to do this we need to add reentrant locking, I consider accidental deadlocks to be worse than garbled output. I'd also prefer to override the |
|
It's reasonable to expect people to manage their own locks when they are racing for access to a stream, though it's got a lot less just-workitude. Garbled console output is what I expect from the multithreading gods. If Rust gets reentrant locks it would be backwards compatible to add one around the entire thing. |
nagisa
changed the title
|
Reentrant mutexes are here. A try build would be nice. |
|
@bors: try |
|
⌛ Trying commit e542f32 with merge 702b000... |
|
@bors: try |
write_fmt calls write for each formatted field. The default implementation of write_fmt is used, which will call write on not-yet-locked stdout (and write locking after), therefore making print! in multithreaded environment still interleave contents of two separate prints. I’m not sure whether we want to do this change, though, because it has the same deadlock hazard which we tried to avoid by not locking inside write_fmt itself (see [this comment](https://github.com/rust-lang/rust/blob/80def6c2447d23a624e611417f24cf0ab2a5a676/src/libstd/io/stdio.rs#L267)). Spotted on [reddit]. cc @alexcrichton [reddit]: http://www.reddit.com/r/rust/comments/31comh/println_with_multiple_threads/
|
💔 Test failed - try-mac |
|
@bors: try |
write_fmt calls write for each formatted field. The default implementation of write_fmt is used, which will call write on not-yet-locked stdout (and write locking after), therefore making print! in multithreaded environment still interleave contents of two separate prints. I’m not sure whether we want to do this change, though, because it has the same deadlock hazard which we tried to avoid by not locking inside write_fmt itself (see [this comment](https://github.com/rust-lang/rust/blob/80def6c2447d23a624e611417f24cf0ab2a5a676/src/libstd/io/stdio.rs#L267)). Spotted on [reddit]. cc @alexcrichton [reddit]: http://www.reddit.com/r/rust/comments/31comh/println_with_multiple_threads/
|
💔 Test failed - try-win-64 |
| } | ||
|
|
||
| #[unstable(feature = "reentrant_mutex", reason = "new API")] | ||
| impl<'mutex, T> DerefMut for ReentrantMutexGuard<'mutex, T> { |
There was a problem hiding this comment.
Unfortunately I believe this is not sound to implement DerefMut for these guards. For example I believe that this is then allowed in safe code:
let a = ReentrantMutex::new(4);
let mut b = a.lock().unwrap();
let mut c = a.lock().unwrap();
let ptr1 = &mut *b;
let ptr2 = &mut *c;
// ptr1/ptr2 are mutable aliasesThere was a problem hiding this comment.
Uh, this hurts a lot, since mutable references are exactly what most stdio methods want and alternatives I’ve considered so far:
- with_mut_ref(&self, fucn: fn(&mut T) -> R) -> R;
- as_mut_ptr(&self) -> *mut T;
are both insufficient. First one prevents aliasing reliably by disallowing to enclose over the environment, at the same time making the whole scheme as useless as it is safe; and the second one allows keeping pointer to the data after the lock expires.
I fear it might be not viable to have ReentrantMutex which protects data inside itself…
There was a problem hiding this comment.
I think that it basically means that to use a reentrant mutex safely you also need to likely have a RefCell in play somewhere. For example even with a function that has no environment you could still do something like creating a cycle of Arc pointers, so T holds a reference to the reentrant mutex allowing you to access it through the &mut T pointer the function pointer is given.
There was a problem hiding this comment.
Err, indeed. I, for some reason, was sure panicking shouldn’t be done here, and dismissed RefCell without considering it enough. It sounds like a most plausible thing to do here.
nagisa
force-pushed
the
print-locking
branch
2 times, most recently
from
f0d46f3
to
a28e18f
Compare
| pub use self::poison::{PoisonError, TryLockError, TryLockResult, LockResult}; | ||
| pub use self::remutex::{ReentrantMutex, ReentrantMutexGuard}; |
There was a problem hiding this comment.
After some chatting with @aturon, we think it may actually be best for now to not expose these publicly for now. Could you move the remutex.rs file to sys_common and use it from there instead?
|
Looking great to me! r=me after moving to |
nagisa
force-pushed
the
print-locking
branch
3 times, most recently
from
55f19d0
to
48877bf
Compare
|
r+? @alexcrichton |
| @@ -67,3 +68,45 @@ impl Mutex { | |||
| debug_assert!(r == 0 || r == libc::EINVAL); | |||
| } | |||
| } | |||
|
|
|||
| pub struct ReentrantMutex { inner: Box<UnsafeCell<ffi::pthread_mutex_t>> } | |||
There was a problem hiding this comment.
Could you leave a FIXME here to remove the Box at some point? Right now it unfortunately boxes twice (once at the common layer and once here as well). Fine for now, just want to make sure we don't forget to remove it at some point (these are supposed to be the lowest level abstractions).
There was a problem hiding this comment.
While you're at it, could you also leave a comment about why it's boxed?
|
Looks great to me, thanks! Could you add a test as well? Something along the lines of:
|
|
Done. EDiT: no it is not, didn’t notice the comment about FIXME EDIT: Yes, done. |
|
⌛ Testing commit 8e397b5 with merge 1bdcdf3... |
|
💔 Test failed - auto-mac-32-opt |
|
@bors: retry um, what? |
|
⌛ Testing commit 8e397b5 with merge a5e1d04... |
|
💔 Test failed - auto-mac-32-opt |
write_fmt calls write for each formatted field. The default implementation of write_fmt is used, which will call write on not-yet-locked stdout (and write locking after), therefore making print! in multithreaded environment still interleave contents of two separate prints. This patch implements reentrant mutexes, changes stdio handles to use these mutexes and overrides write_fmt to lock the stdio handle for the whole duration of the call.
|
⌛ Testing commit 45aa6c8 with merge ff80477... |
write_fmt calls write for each formatted field. The default implementation of write_fmt is used, which will call write on not-yet-locked stdout (and write locking after), therefore making print! in multithreaded environment still interleave contents of two separate prints. I’m not sure whether we want to do this change, though, because it has the same deadlock hazard which we tried to avoid by not locking inside write_fmt itself (see [this comment](https://github.com/rust-lang/rust/blob/80def6c2447d23a624e611417f24cf0ab2a5a676/src/libstd/io/stdio.rs#L267)). Spotted on [reddit]. cc @alexcrichton [reddit]: http://www.reddit.com/r/rust/comments/31comh/println_with_multiple_threads/
|
Great stuff! Are there any future plans re: stabilizing |
write_fmt calls write for each formatted field. The default implementation of write_fmt is used,
which will call write on not-yet-locked stdout (and write locking after), therefore making print!
in multithreaded environment still interleave contents of two separate prints.
I’m not sure whether we want to do this change, though, because it has the same deadlock hazard which we tried to avoid by not locking inside write_fmt itself (see this comment).
Spotted on reddit.
cc @alexcrichton