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

Reorganize subcrates for futures 0.3 #1071

Closed
MajorBreakfast opened this issue Jul 9, 2018 · 9 comments
Closed

Reorganize subcrates for futures 0.3 #1071

MajorBreakfast opened this issue Jul 9, 2018 · 9 comments

Comments

@MajorBreakfast
Copy link
Contributor

We should reevaluate which subcrates contain what parts of the futures crate. I think that we should move some things around prior to releasing an alpha to get some consistency.

  • Currently it seems a bit inconsistent that the futures_core crate contains Future and Stream and also ReadyFuture, CoreFutureExt, ContextExt and AtomicWaker, but the io traits and Sink have their own crates. I think we should establish guidelines that define what kind of things belong in futures_core and discuss whether futures_core should exist at all.
  • Should there be a separation between executor and task? I'd say yes, but I'm not 100% sure. In any case some stuff like LocalExecutor should be moved into task(as a trait). It's weird that LocalExecutor is a struct and Executor a trait. That's unlike FutureObj/LocalFutureObj and Waker/LocalWaker where always both are a trait or a struct respectively.
  • I would like to remove the top level exports. See this comment for a detail explanation why I think that would improve things. @Nemo157 and @seanmonstar raised some concerns and I would like to have further debate about this.
  • The futures-util crate feels like a kitchen sink. I've counted the lines of code in the src/ directories of each crate and the utils crate contains three quarters of the total code (76%).
    - `futures/src` 485 loc
    - `futures-core/src` 849 loc
    - `futures-executor/src` 1095 loc
    - `futures-io/src`  372 loc
    - `futures-sink/src` 364 loc
    - `futures-util/src` 10263 loc
    

@Nemo157
@carllerche
@seanmonstar
@withoutboats
@aturon
@cramertj

@Nemo157
Copy link
Member

Nemo157 commented Jul 9, 2018

I think we should establish guidelines that define what kind of things belong in futures_core and discuss whether futures_core should exist at all.

I believe the guideline is "things that will (hopefully) never change". futures-core is intended to be the root of interoperability, libraries public APIs should only expose the types from this crate. This allows the other crates to bump version numbers as needed without breaking interoperability between dependent crates, crate foo can be using futures-util 0.14.5 and crate bar can be using futures-util 0.52.6 while still passing futures-core 0.3.4 Future objects to each other.

but the io traits and Sink have their own crates

Sink is a bit of a special case. I believe it's intended to be part of futures-core in the future, but the API design is not quite there yet. So it's being kept in a separate crate until the API stabilizes (see this part of the futures 0.2 RFC).

The futures-io traits require some more dependencies that futures-core doesn't, I would also assume they're not 100% stable yet, maybe once tokio migrates over to them they could be proven enough to become part of futures-core?

I wouldn't be surprised if over time bits and pieces migrate over to futures-core and eventually that crate is promoted to being just futures and the sub-crates all become re-exports from it.


In any case some stuff like LocalExecutor should be moved into task(as a trait).

What about

trait StaticExecutor<F: Future> {
    fn spawn(&mut self, future: F) -> Result<(), SpawnError>;
}

(static as compared with dynamic trait object for the current executor trait). I think there's a few different executor traits that should be played around with; there's no rush to push these traits into the core library until they have been explored well. The only reason the main Executor trait needs to be in the core library is because we want Context to implicitly carry one, other executor variants will be explicitly passed in where needed so so can their types.

I've only really been skimming the current implementation stuff, so I'm actually unsure why the LocalExecutor stuff is in std at this point, the latest "futures and task system" RFC doesn't mention it at all; is there some fundamental reason that can't be part of futures-executor?


The futures-util crate feels like a kitchen sink.

I've wondered in the past whether futures-io-util should be split out to a separate crate, but I don't think it's worth it.

A more charitable way to describe this crate would be the Swiss Army Knife of futures, it's just all the basic low-level functions that you might need when chopping up, tearing apart, sticking together bits and pieces to build up your custom future. In a lot of respects it's similar to https://doc.rust-lang.org/nightly/std/iter/index.html, but repeated 3 times for the 3 fundamental traits of futures. It really is the main meat of the futures crate, and I don't think there's much of a problem with that.


An associated question is whether libraries should be recommended to import the futures facade or the individual crates. That might affect the organization somewhat.

@cramertj
Copy link
Member

cramertj commented Jul 9, 2018

@MajorBreakfast Thanks for bringing this up! @aturon and I had discussed some possible changes to organization as well. I basically agree with everything that @Nemo157 said-- for me, the separation is all about stability tracking.

futures-core shouldn't have anything that we could forsee changing in the near future. futures-io could probably be joined with futures-core but it's still a bit uncertain since it's not clear whether traits such as AsyncRead should be changed to use self: PinMut<Self> down the road. futures-sink is far less stable since there are many ongoing conversations about how to revamp and improve the Sink trait (e.g. to handle sending borrowed values). futures-channel is more stable than futures-sink, but it, too, is the topic of ongoing conversation for improvements (mostly performance related). futures-util is a grab-bag of everything else that we think users might find convenient to use, but which wouldn't commonly be part of the public API of a crate. This means that we can make breaking releases of futures-util without causing major ecosystem disruption.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jul 9, 2018

I believe the guideline is "things that will (hopefully) never change". futures-core is intended to be the root of interoperability

@Nemo157 That's actually a pretty good guideline!

An associated question is whether libraries should be recommended to import the futures facade or the individual crates

@Nemo157 Library authors should probably not use the facade crate. Otherwise the interoperability can't work. I think we should recommend this clearly.


  • What do you think about renaming "futures-util" to "futures-main"? "util" sounds a bit like the sideshow, not the main event.
  • What about "futures-core-future", "futures-core-stream", "futures-core-io", etc.? The current names "futures-io" and "futures-sink" don't really convey that these crates contain only definitions.

I would like to clean out futures_core and only leave the essentials. This is what I suggest:

  • AtomicWaker should be moved out. Maybe into futures-executor?
  • CoreFutureExt should be merged into FutureExt
  • ContextExt should be moved into futures-util
  • ReadyFuture should be moved into futures-util
  • Stream impl for Either should be removed? We already had to remove the one for Future when Future was moved to libcore. (It's still in there, but commented out)
  • FutureOption should be moved into futures_util
  • Move the macros into futures-macro

This leaves: The core reexports, TryFuture, Stream & TryStream

@Nemo157
Copy link
Member

Nemo157 commented Jul 9, 2018

@Nemo157 Library authors should probably not use the facade crate. Otherwise the interoperability can't work. I think we should recommend this clearly.

Surprisingly it still does, as an example:

foo
├─ bar
│  └─ futures 0.3.0
│     └─ futures-core 0.3.0
└─ baz
   └─ futures 0.5.0
      └─ futures-core 0.3.0

Even though bar and baz are using different versions of futures the Future traits that they are using are both re-exported from futures-core 0.3, so are the same trait. foo can pass futures from bar to baz and everything will work fine. This almost certainly defeats my thoughts on using a trait alias in futures, that would tie the trait alias to FutureExt and break this interoperability.

Once we get the ability to distinguish between public and private dependencies then it's definitely better for libraries to depend on the subcrates, futures-core should be a public dependency and futures-util should be a private dependency, then you will have compile time checking to ensure you don't leak futures-util items into your public API.

@MajorBreakfast
Copy link
Contributor Author

@Nemo157 This is great!

Looking at this though I think that Stream should get its own crate "futures-core-stream". We can keep futures-core with Future and task because they're both in libcore, but since Stream isn't officially stable, it should maybe not be included in the core crate. There's still the possibility that it could change. And I think we'll probably see some minor changes when async generators start becoming a thing.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jul 10, 2018

Here's the layout I propose:

  • futures: Facade
  • futures-core: Reexports libcore's future and task module, contains nothing else
  • futures-core-stream: Stream definition
  • futures-core-sink: Sink definition
  • futures-core-io: AsyncRead, AsyncWrite
  • futures-channel
  • futures-executor: Now also contains AtomicWaker
  • futures-util: Let's stick with the current name. Now also contains: Ready, FutureOption, pin & poll macros, AtomicWaker

Rationale:

  • All definition creates get a name that starts with futures-core. That way they can be easily recognized. Additionally the word "core" suggests that they contain only the essentials. The old names like "future-sink" do not suggest that properly.
  • Stream, Sink and the io traits can be versioned independetly because they live in different crates. None of them has been formally stabilized yet, so they should not live in futures-core.

Edit: Atomic waker is used by FuturesUnordered

@cramertj
Copy link
Member

cramertj commented Jul 10, 2018

Some thoughts:

futures-core: Reexports libcore's future and task module, contains nothing else

This seems unnecessary to me, since it would be versioned and used identically to std itself.

futures-core-stream: Stream definition

This is basically the current futures-core, modulo some small helpers that are isolated and stable.

futures-core-sink/io

I'm not sold on the name-move. -core is intended to indicate that it's a stable piece which isn't likely to change in the future, which isn't true of Sink, at least. If things are reasonably stable, they should be moved into futures-core so that we don't incur the complexity cost of a separate crate.

TL;DR My preference is to keep things as they are for now, although I don't care too much about where things like Ready, FutureOption, and AtomicWaker go-- it seems fine to me to put them in futures-util, though I think they're stable enough that we could keep them in -core.

@seanmonstar
Copy link
Contributor

I agree that keeping things the way they are seems fine. I don't see a compelling argument for the churn.

@MajorBreakfast
Copy link
Contributor Author

The changes that we agreed on were implemented in #1078 The previously bloated futures-core crate is lean and mean with only the core definitions remaining in it.

The only thing that will be a problem when Stream is moved to libcore is the Either impl. But, we can keep it for now.

I think this issue can now be closed. I'll keep it open for one more day to see if there're any more comments and close it then if there are no objections.

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

No branches or pull requests

4 participants