Skip to content
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

Introduce LocalTaskObj #1046

Merged
merged 1 commit into from
Jun 27, 2018
Merged

Conversation

MajorBreakfast
Copy link
Contributor

@MajorBreakfast MajorBreakfast commented Jun 25, 2018

  • Introduce LocalTaskObj and integrate it into LocalPool
  • Export all the stuff form core::task under futures::task (some was previously exported under futures::executor) This is probably just a start. There're probably still some inconsistencies in the exports.
  • This PR makes the tests in futures-executor/tests/local_pool.rs work

@cramertj
Copy link
Member

@MajorBreakfast Pinged you on discord about this. I think, rather than a separate trait, I'd prefer to remove the Send bound on the UnsafeTask trait, and only introduce it in TaskObj::new. I also think I'd prefer for LocalTaskObj to live in core so that the conversion from TaskObj to LocalTaskObj doesn't depend on unstable representation details of std.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jun 25, 2018

I agree. It would then be very similar to how we do it with the UnsafeWake trait. The only thing different is that UnsafeWake needs bounds for Sync and Send because of the cloning as Waker. For this here however we, as you say, shouldn't add bounds to the trait, so that PinBox with a Future<Output = ()> + 'static can implement it

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jun 25, 2018

Do we want to add SpawnLocalObjError to libcore as well? It's not absolutely necessary because the fields are public. (But we need it somewhere because SpawnObjError can't store a LocalTaskObj)

@cramertj
Copy link
Member

@MajorBreakfast Hm, good question! I think either way is fine-- these are all going to be deprecated once unsized-rvalues + arbitrary-self-type-object-safety are fixed, so I don't think it matters particularly much either way.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jun 25, 2018

We will still need need the distinction for between LocalSpawnError and SpawnError (just without Obj in the name) because of the Send difference.

If you let me pick, then I'd put them into core because this way we can use transmute for conversion between them. LocalPools's spawn_obj calls spawn_local_obj internally, so this conversion is on a hot code path.

@cramertj
Copy link
Member

cramertj commented Jun 25, 2018

@MajorBreakfast In the long term, I figured we wouldn't have LocalSpawnObjError or LocalSpawnError, only the current SpawnErrorKind. But either way is fine w/ me.

@MajorBreakfast
Copy link
Contributor Author

Here is the PR that puts LocalTaskObj into libcore rust-lang/rust#51814

SpawnError gives us the task back when spawning isn't possible. This makes it possible to spawn it somewhere else. I don't think that it will go away. (And LocalSpawnError does the same for non-Send tasks)

@cramertj
Copy link
Member

@MajorBreakfast It will have to go away once spawning goes through unsized rvalues, since there's no way to hand them back.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jun 26, 2018

That's a bummer.

So, this won't work? I thought the last element of a struct can be unsized?

pub struct SpawnError {
    pub kind: SpawnErrorKind,
    pub future: dyn Future<Output = ()> + Send,
}

Edit: I just looked at the RFC again. Functions will have to return a sized type. Then it's clear that it can't work

@MajorBreakfast
Copy link
Contributor Author

The new nightly which now includes LocalTaskObj in libcore requires some fixes to the futures crate. Therefore this is now blocked on #1049

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jun 27, 2018

I've updated the PR to use core::task::LocalTaskObj

@cramertj
Copy link
Member

Nice! This is great!

@cramertj cramertj merged commit eb10d6e into rust-lang:0.3 Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants