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

Remove spawning from task::Context #54339

Merged
merged 1 commit into from
Sep 23, 2018
Merged

Remove spawning from task::Context #54339

merged 1 commit into from
Sep 23, 2018

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Sep 19, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2018
@seanmonstar
Copy link
Contributor

With the remove of FutureObj and co, unless arbitrary_self_types has made Future object-safe, there's no longer a way to have Future trait objects, right?

@rust-highfive

This comment has been minimized.

@Nemo157
Copy link
Member

Nemo157 commented Sep 19, 2018

Is the plan to provide a Spawn trait and FutureObj etc. in futures, or just drop them from the "core" abstractions?

@cramertj
Copy link
Member Author

cramertj commented Sep 19, 2018

@seanmonstar

With the remove of FutureObj and co, unless arbitrary_self_types has made Future object-safe, there's no longer a way to have Future trait objects, right?

We'll continue to provide these types inside the futures crate, but they no longer need to be part of the standard library.

@Nemo157

Is the plan to provide a Spawn trait and FutureObj etc. in futures, or just drop them from the "core" abstractions?

Yup-- everything that is being removed from here will reappear in the futures crate.

@aturon
Copy link
Member

aturon commented Sep 20, 2018

This looks great, nice simplification.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 20, 2018

📌 Commit 1b00f0b has been approved by aturon

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2018
@Thomasdezeeuw
Copy link
Contributor

Thomasdezeeuw commented Sep 21, 2018

Isn't this a bit quick since the discussion is still going in rustasync/team#56? I'm for this change, but not everbody is.

@cramertj
Copy link
Member Author

@Thomasdezeeuw The point of having the current std API is to experiment with the intended design-- any design (even the current one) needs to go through a real RFC before stabilization. I think the conversation in rustasync/team#56 has turned up enough motivating factors that we should give this approach a shot. If it doesn't work out or the RFC process resolves to a different design, then we can change it. If you think there's a solid reason we shouldn't do this (to the point that it's not worth trying) please leave a comment on rustasync/team#56 so we can take that into account!

@Ekleog
Copy link

Ekleog commented Sep 23, 2018

@cramertj I think I put forward some arguments against rushing this in rustasync/team#56 , that were only answered by “don't do this, change your API” (which I don't really consider a valid resolution). Basically, this without any solution to rustasync/team#7 is breaking the (my? maybe I'm the only one here who actually has a use case for access to the default spawner from library code…) world without any other way to do the same thing.

On the other hand, this change can happen at any time until the RFC. Is there really a need to rush this forward before solving rustasync/team#7?

@bors
Copy link
Contributor

bors commented Sep 23, 2018

⌛ Testing commit 1b00f0b with merge 2287a7a...

bors added a commit that referenced this pull request Sep 23, 2018
@bors
Copy link
Contributor

bors commented Sep 23, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 2287a7a to master...

@bors bors merged commit 1b00f0b into rust-lang:master Sep 23, 2018
@cramertj cramertj deleted the no-cx branch September 24, 2018 17:45
@cramertj
Copy link
Member Author

Is there really a need to rush this forward before solving rustasync/team#7?

@Ekleog IMO one of the best steps we can take towards solving rustasync/team#7 is to actually get experience using both APIs, which this change allows us to do. Additionally, I'd like to open an RFC for futures-in-std soon, and it'd be best if we could say we'd successfully tested and been using the same API as the one being RFC'd.

@Ekleog
Copy link

Ekleog commented Sep 25, 2018

@cramertj I'm not really sure I understand whether you already meant it, but… it sounds to me like the potential hooks mentioned by rustasync/team#7 would likely need to be implemented in the Executor trait? Which would mean a futures-in-std RFC would still be blocked on experimenting with the outcome of rustasync/team#7 anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants