-
Notifications
You must be signed in to change notification settings - Fork 732
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
Custom readiness queue #371
Conversation
interest: Cell<EventSet>, | ||
|
||
// Poll opts | ||
opts: Cell<PollOpt>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally pretty nervous about the direction this abstraction is headed in. As is it seems like it's incredibly difficult to get right in terms of preventing race conditions and other undefined behavior, and it's barely leveraging Rust types for managing this unsafety. Specifically this structure is conglomeration of:
- Raw unsafe pointers
- non-thread-safe
Cell
types - threadsafe atomics
This, combined with the fn inner_mut(&self) -> &mut ReadinessQueueInner
signature below, make this very difficult to determine whether it's actually safe or not.
I think this would benefit quite a bit from extracting some of the unsafe components here to standalone and isolated features with safe interfaces, or very very clear unsafe interfaces with documentation as to what contracts need to be upheld. For example next_all_notes
and prev_all_nodes
looks a lot like a doubly linked list? Does the standard library's LinkedList
not suffice here because atomics are necessary? Some other reason?
Similarly if there's a set of fields that are only accessed by a Poll
on each node, then this may benefit from a more concrete interface to indicate such. For example I would expect something like:
struct PollFields { ... }
struct ReadinessNode {
poll_fields: UnsafeCell<PollFields>,
}
impl ReadinessNode {
fn poll_fields<'a>(&'a self, _proof: &'a Poll) -> &'a PollFields { /* ... */ }
}
Basically something like that where the API itself is very difficult to get wrong if properly used. That should help anyone else working around here or perhaps any future refactorings as well.
3ac3056
to
0bc141c
Compare
88494d5
to
ad27342
Compare
Looks like CI is green. I'm hoping to merge this! |
if 0 == self.inner.pending.fetch_add(1, Ordering::Acquire) { | ||
// Toggle readiness to readable | ||
if let Some(set_readiness) = self.inner.set_readiness.as_ref() { | ||
// TODO: Don't ignore result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO is handled, right?
a926d86
to
7d43e9e
Compare
LGTM |
impl<T> StdSender<T> { | ||
pub fn send(&self, t: T) -> Result<(), SendError<T>> { | ||
match *self { | ||
StdSender::Bounded(ref tx) => tx.send(t).map_err(SendError::from), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be using try_send
?
This commit adds the ability to implement `Evented` and use it with a `Poll` instance as expected. This works by having a new readiness queue that sits next to the OS specific selector. Future work includes: * Implementing Timer using the new queue * Migrate Windows support to use the custom queue * Have `Poll::poll()` take `&self` * More documentation
48fbd5b
to
6ca6326
Compare
WIP, not ready for merging. This PR will be the first step towards #360