Fix feedback from libstd PR, part 1 #140
Conversation
| /// from any other memory address than the one they were located at when they | ||
| /// were initialized. As such, it's UB to call any unsafe method on | ||
| /// `ThreadParkerT` if the implementing instance has moved since the last | ||
| /// call to any of the unsafe methods. |
There was a problem hiding this comment.
It's kind of sad how this one implementation spreads unsafe to all platforms and the API in general. If the generic Unix implementation just boxed the mutex/condvar I think it could become safe, right? It might lose some performance, but that can be measured so we have some numbers. If the difference is not too large, maybe it would be sane to make the entire thread parker API safe in this way?
We can probably move more platforms away from the generic Unix parker and to better, more specific parkers. For example, could not Android use the same futex based one as Linux? Is there nothing more modern than pthread mutexes for BSD?
There was a problem hiding this comment.
I believe Android should already be using futexes? It uses the Linux kernel so target_os should be linux, right?
There was a problem hiding this comment.
No, it has target_os = "android". See here: https://github.com/rust-lang/libc/blob/master/src/unix/mod.rs#L1141
There was a problem hiding this comment.
Maybe use Pin to guarantee that self is not moved?
There was a problem hiding this comment.
@bjorn3 Do you have an example on how to do that? I don't know a way to express that without introducing a layer of indirection between the code holding the ThreadParker and the actual pthread C structs.
There was a problem hiding this comment.
Note that there's a lot of UB scenarios with pthread utilities that are beyond movement (that's one of the major reasons for moving to parking_lot!). I suspect that this is is probably going to have to stay unsafe even if we were to box primitives, but it would be good to document all the unsafe guarantees we know of at least that must be upheld.
While it's unfortuante that the unsafe leaks to other platforms it's also sort of the reality of writing portable code. You're often limited by the lowest common denominator.
faern
force-pushed
the
fix-feedback-from-libstd-1
branch
3 times, most recently
from
508c842 to
33b1430
Compare
core/src/thread_parker/mod.rs
Outdated
| } | ||
|
|
||
| cfg_if! { | ||
| if #[cfg(all(has_sized_atomics, target_os = "linux"))] { |
There was a problem hiding this comment.
You might as well add android here as well. Futex should work since Android also uses Linux.
There was a problem hiding this comment.
Sure. I'm not able to test this myself, but now it's added.
| fn new() -> Self; | ||
|
|
||
| /// Prepares the parker. This should be called before adding it to the queue. | ||
| unsafe fn prepare_park(&self); |
There was a problem hiding this comment.
Could this and the methods below have expanded documentation as to why they're unsafe? (e.g. what has to be upheld to actually call them safely)
There was a problem hiding this comment.
I'll leave most of the requested documentation updates to @Amanieu. So that will likely be done in a separate PR.
faern
force-pushed
the
fix-feedback-from-libstd-1
branch
from
14da63e to
3d5a9c3
Compare
faern
force-pushed
the
fix-feedback-from-libstd-1
branch
from
3d5a9c3 to
6607fd3
Compare
|
@Amanieu Fixed the broken stuff. I think this is ready for review/merge now. |
|
Sorry that it took me so long to review this, but it looks mostly fine. |
Here I address some of the fairly low hanging fruit from faern#1
unlock_bucket_pair. https://github.com/faern/parking_lot/pull/1/files#r283107487impl Traitinstead of a bunch of generics around the park/unpark calls. https://github.com/faern/parking_lot/pull/1/files#diff-f0c71d8c95c3701fb57bafd0a8198201R625#[inline(never)]. https://github.com/faern/parking_lot/pull/1/files#diff-936abe3ecea632b68dddc509e87c7c39R142ThreadParkerandUnparkHandletypes. So we have a single place to stick the documentation. And to better ensure all implementers have the exact same API. https://github.com/faern/parking_lot/pull/1/files#r283155912cc @alexcrichton