Skip to content

Improve core::task::TaskObj #51532

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 13, 2018
Merged

Improve core::task::TaskObj #51532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions src/liballoc/boxed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use core::marker::{Unpin, Unsize};
use core::mem::{self, PinMut};
use core::ops::{CoerceUnsized, Deref, DerefMut, Generator, GeneratorState};
use core::ptr::{self, NonNull, Unique};
use core::task::{Context, Poll, UnsafePoll, TaskObj};
use core::task::{Context, Poll, UnsafeTask, TaskObj};
use core::convert::From;

use raw_vec::RawVec;
Expand Down Expand Up @@ -933,7 +933,7 @@ impl<'a, F: ?Sized + Future> Future for PinBox<F> {
}

#[unstable(feature = "futures_api", issue = "50547")]
unsafe impl<F: Future<Output = ()> + Send + 'static> UnsafePoll for PinBox<F> {
unsafe impl<F: Future<Output = ()> + Send + 'static> UnsafeTask for PinBox<F> {
fn into_raw(self) -> *mut () {
PinBox::into_raw(self) as *mut ()
}
Expand All @@ -952,13 +952,13 @@ unsafe impl<F: Future<Output = ()> + Send + 'static> UnsafePoll for PinBox<F> {
#[unstable(feature = "futures_api", issue = "50547")]
impl<F: Future<Output = ()> + Send + 'static> From<PinBox<F>> for TaskObj {
fn from(boxed: PinBox<F>) -> Self {
TaskObj::from_poll_task(boxed)
TaskObj::new(boxed)
}
}

#[unstable(feature = "futures_api", issue = "50547")]
impl<F: Future<Output = ()> + Send + 'static> From<Box<F>> for TaskObj {
fn from(boxed: Box<F>) -> Self {
TaskObj::from_poll_task(PinBox::from(boxed))
TaskObj::new(PinBox::from(boxed))
}
}
29 changes: 15 additions & 14 deletions src/libcore/task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

use fmt;
use ptr::NonNull;
use future::Future;
use mem::PinMut;

/// Indicates whether a value is available or if the current task has been
/// scheduled to receive a wakeup instead.
Expand Down Expand Up @@ -455,8 +457,8 @@ pub trait Executor {
/// `Box<Future<Output = ()> + Send>`.
pub struct TaskObj {
ptr: *mut (),
poll: unsafe fn(*mut (), &mut Context) -> Poll<()>,
drop: unsafe fn(*mut ()),
poll_fn: unsafe fn(*mut (), &mut Context) -> Poll<()>,
drop_fn: unsafe fn(*mut ()),
}

impl fmt::Debug for TaskObj {
Expand All @@ -467,7 +469,6 @@ impl fmt::Debug for TaskObj {
}

unsafe impl Send for TaskObj {}
unsafe impl Sync for TaskObj {}
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? We don't particularly need it, but it's totally sound AFAICT, and it will give the correct behavior were we to add more non-mutating methods in the future (in addition to the current Debug::fmt).

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 can put it in again, but is it safe? trait UnsafeTask: Send + 'static <-- no Sync

Copy link
Contributor Author

@MajorBreakfast MajorBreakfast Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

According to the comment above TaskObj is supposed to be approximately equivalent to Box<Future<Output = ()> + Send> which has also no Sync bound.

Copy link
Member

Choose a reason for hiding this comment

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

UnsafeTask only has &mut methods-- those are only used for mutating the inner task. Since no &self methods are called, it's trivially Sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TaskObj is just a workaround for Box<Future<Output = ()> + Send + 'static> which throws an error, but if it worked, it wouldn't be be Sync. I think the workaround should match what we really want but can't get as much as possible. IMO it feels wrong to implement Sync. I agree with your reasoning why it can be considered safe, though.

Here's the error for Box<Future<Output = ()> + Send + 'static>:

let boxed: Box<Future<Output = ()> + Send + 'static> = ...
// error[E0038]: the trait `std::future::Future` cannot be made into an object
// note: method `poll` has a non-standard `self` type

Also a playground example that shows Box<Future<Output = ()> + Send + 'static> is not Sync

I believe the more conservative approach would be to leave it out.

Copy link
Member

@cramertj cramertj Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That is certainly the more conservative approach :).

I'm fine with leaving it out if you feel strongly about it-- I don't think it really matters significantly either way. Currently the only &self method is Debug::fmt, which is thread-safe, but if we someday decide to add a thread-locked &self method you're right that we wouldn't be able to do so.

Copy link
Contributor Author

@MajorBreakfast MajorBreakfast Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No strong feelings, because, as you say, it's safe. My recommendation is, however, to leave it out.

For adding impl Sync: We can because it's safe.
Against adding impl Sync: More conservative, we don't need it, and it approximates Box<Future...> which is !Sync more closely
=> My conclusion: Don't add it


/// A custom implementation of a task trait object for `TaskObj`, providing
/// a hand-rolled vtable.
Expand All @@ -478,7 +479,7 @@ unsafe impl Sync for TaskObj {}
/// The implementor must guarantee that it is safe to call `poll` repeatedly (in
/// a non-concurrent fashion) with the result of `into_raw` until `drop` is
/// called.
pub unsafe trait UnsafePoll: Send + 'static {
pub unsafe trait UnsafeTask: Send + 'static {
/// Convert a owned instance into a (conceptually owned) void pointer.
fn into_raw(self) -> *mut ();

Expand All @@ -504,30 +505,30 @@ pub unsafe trait UnsafePoll: Send + 'static {
impl TaskObj {
/// Create a `TaskObj` from a custom trait object representation.
#[inline]
pub fn from_poll_task<T: UnsafePoll>(t: T) -> TaskObj {
pub fn new<T: UnsafeTask>(t: T) -> TaskObj {
TaskObj {
ptr: t.into_raw(),
poll: T::poll,
drop: T::drop,
poll_fn: T::poll,
drop_fn: T::drop,
}
}
}

impl Future for TaskObj {
type Output = ();

/// Poll the task.
///
/// The semantics here are identical to that for futures, but unlike
/// futures only an `&mut self` reference is needed here.
#[inline]
pub fn poll_task(&mut self, cx: &mut Context) -> Poll<()> {
fn poll(self: PinMut<Self>, cx: &mut Context) -> Poll<()> {
unsafe {
(self.poll)(self.ptr, cx)
(self.poll_fn)(self.ptr, cx)
}
}
}

impl Drop for TaskObj {
fn drop(&mut self) {
unsafe {
(self.drop)(self.ptr)
(self.drop_fn)(self.ptr)
}
}
}
Expand Down