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

Move Spawn/AtomicWaker/ArcWake/FutureObj to futures-task #1925

Closed
wants to merge 8 commits into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Oct 31, 2019

This is the idea I mentioned in #1893 (comment), and address some suggestions of #1893 (comment):

#1893 (comment)

Also, I was thinking of adding a new crate as a solution for #1830 (comment).
Personally, I think it might be a good idea to add a crate that provides "items contained in the futures::task module except for *Ext trait".

#1893 (comment)

  • futures-core
    • Remove Spawn, LocalSpawn, SpawnError, and put them in futures-executor somehow. Separate Spawn/LocalSpawn from futures-core? #1830
    • Remove FutureObj and LocalFutureObj (they seem only needed for Spawn and friends?).
    • Remove AtomicWaker. It's only exposed in a doc(hidden) task::__internal module, but it doesn't seem like we need to compile it if only after the core traits.

Closes #1830

r? @cramertj
cc @seanmonstar @Nemo157

@cramertj
Copy link
Member

This is great! What would you think about moving AtomicWaker back so that we can drop the dependency on futures-task from futures-channel? I think that FutureObj and Spawn are fairly unstable and possibly not long for this world, and it'd be a shame to build in a dependency on them from channels (though maybe it's not a huge deal).

cc @seanmonstar -- do you have thoughts here? Being able to share AtomicWaker seems good, and I think the way it's done currently leaves us lots of room to adapt in the future. I don't see much cost to having it shipped with futures-core as an unstable internal implementation detail.

@seanmonstar
Copy link
Contributor

My personal feeling is that making futures-core as light as possible is a good goal. On one hand, AtomicWaker isn't that much code, but on the other, for example, I have no use for it in many things that rely on futures-core, so it'd seem nice to not need to compile it.

@cramertj
Copy link
Member

it'd seem nice to not need to compile it.

Makes sense! I'm skeptical that it's introducing significant compile-time cost, and it seems likely that other things in an async project would potentially make use of it (e.g. anything using futures or futures-util). That said, since it isn't part of futures-core's stable API surface it seems fine to revisit in the future. I'd like to make as few commitments now as possible to give futures-rs room to evolve, which makes me concerned about offering AtomicWaker in futures-task or having futures-channel depend on futures-task.

@taiki-e
Copy link
Member Author

taiki-e commented Oct 31, 2019

What would you think about moving AtomicWaker back so that we can drop the dependency on futures-task from futures-channel?

I don't have a strong opinion. Also, by adding a new unstable feature like 'internal', you can compile it only when you need it. However, since stable crates should use futures-task, I don't think the benefits from this are big.

@cramertj
Copy link
Member

cramertj commented Nov 1, 2019

Primarily I want to leave room for dropping the futures-task dependency from futures-channel in the future, which I'm concerned this wouldn't allow since this change would make futures-channel enable features in futures-task.

@carllerche
Copy link
Member

Fwiw, long term, I plan to move away from using AtomicWaker in the Tokio channel types. I’ve found that there are more efficient, specialized strategies.

I mention this in case you want to avoid including AtomicWaker until you evaluate whether or not the futures types will use it long term.

@cramertj
Copy link
Member

cramertj commented Nov 1, 2019

@carllerche Right, I think the right place to expose it is futures-util, which is where it was before this change, and is the crate that carries the loosest stability requirements.

@cramertj cramertj added this to the 0.3 release milestone Nov 1, 2019
@taiki-e
Copy link
Member Author

taiki-e commented Nov 3, 2019

@cramertj

Primarily I want to leave room for dropping the futures-task dependency from futures-channel in the future, which I'm concerned this wouldn't allow since this change would make futures-channel enable features in futures-task.

In this PR's approach, once we find an alternative to AtomicWaker, futures-channel can remove the dependency on futures-task.

Also, note that when defining AtomicWaker in futures-core, even if futures-channel no longer requires AtomicWaker, AtomicWaker cannot be removed from futures-core because need to maintain compatibility with older versions of futures-channel.

@Matthias247
Copy link
Contributor

Matthias247 commented Nov 4, 2019

Regarding AtomicWaker and futures-core: Wouldn't futures-core be fully no-std compatible, only based on core, and not having any feature flags after those changes? If yes, then it would be great to keep it that way!

I would rather not recommend to expose AtomicWaker publically - purely for avoiding to send out the message that this is the preferred or recommended way to use a Waker API. It's a tool specific for some use-cases, and other use-cases might be better served with assigning Wakers in synchronized blocks (mutexes) that also protected the associated data. If someone wants to go the harder way use atomics in their application they should be fine with having their own copy of this code.

@cramertj
Copy link
Member

cramertj commented Nov 4, 2019

@taiki-e

In this PR's approach, once we find an alternative to AtomicWaker, futures-channel can remove the dependency on futures-task.

I don't believe it can, though, since it enables feature flags in futures-task that wouldn't be present otherwise. If the dep gets dropped, those features will stop being enabled, and users using futures-task directly would see features go away.

I also think that AtomicWaker is not as fundamental as these traits, and is primarily a utility and should only be exposed in futures-util.

@taiki-e
Copy link
Member Author

taiki-e commented Nov 5, 2019

@cramertj
I agree with AtomicWaker should only be exposed in -util. so I've reverted dd664a3.


To keep a record, I'm still concerned about what I said in #1925 (comment):

Also, note that when defining AtomicWaker in futures-core, even if futures-channel no longer requires AtomicWaker, AtomicWaker cannot be removed from futures-core because need to maintain compatibility with older versions of futures-channel.

Given the current maintenance state of futures-channel, I don't think there will be any immediate changes, but I think this should be considered in the next minor (or major) version. (The way I said in #1925 (comment), or something else)

@cramertj
Copy link
Member

cramertj commented Nov 5, 2019

Github seems to be having issues, so I've landed this manually.

@cramertj cramertj closed this Nov 5, 2019
@cramertj
Copy link
Member

cramertj commented Nov 5, 2019

Thanks so much for the PR <3

@taiki-e
Copy link
Member Author

taiki-e commented Nov 5, 2019

By the way, this crate name (futures-task) is reserved by me, so I will hand off to @cramertj.

@taiki-e taiki-e deleted the task branch November 5, 2019 18:16
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.

Separate Spawn/LocalSpawn from futures-core?
5 participants