Improve core::task::TaskObj #51532
Improve core::task::TaskObj #51532
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @cramertj |
| @@ -467,7 +469,6 @@ impl fmt::Debug for TaskObj { | |||
| } | |||
|
|
|||
| unsafe impl Send for TaskObj {} | |||
| unsafe impl Sync for TaskObj {} | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I can put it in again, but is it safe? trait UnsafeTask: Send + 'static <-- no Sync
There was a problem hiding this comment.
According to the comment above TaskObj is supposed to be approximately equivalent to Box<Future<Output = ()> + Send> which has also no Sync bound.
There was a problem hiding this comment.
UnsafeTask only has &mut methods-- those are only used for mutating the inner task. Since no &self methods are called, it's trivially Sync.
There was a problem hiding this comment.
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` typeAlso 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@MajorBreakfast This looks good to me modulo the nit about removing |
|
@bors r+ |
|
📌 Commit 2177378 has been approved by |
bors
added
S-waiting-on-bors
Improve core::task::TaskObj - Rename `UnsafePoll` to `UnsafeTask` to avoid confusion with `Poll` - Rename `TaskObj::from_poll_task()` to `TaskObj::new()` - Rename `TaskObj`'s `poll` and `drop` fields to `poll_fn` and `drop_fn` - Implement `Future` for `TaskObj`. Reason: It's a custom trait object for a future, so it should implement future - Remove `unsafe impl Sync` for `TaskObj`. I don't think we need it. Was this safe? `UnsafeTask` only requires to implement `Send` @cramertj @aturon
|
☀️ Test successful - status-appveyor, status-travis |
|
🥂My first merged "real" Rust PR :-) |
|
@MajorBreakfast That's awesome! Congratulations! |
UnsafePolltoUnsafeTaskto avoid confusion withPollTaskObj::from_poll_task()toTaskObj::new()TaskObj'spollanddropfields topoll_fnanddrop_fnFutureforTaskObj. Reason: It's a custom trait object for a future, so it should implement futureunsafe impl SyncforTaskObj. I don't think we need it. Was this safe?UnsafeTaskonly requires to implementSend@cramertj
@aturon