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

Start futures 0.4 and futures-core 1.0 development #2335

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Feb 13, 2021

Now that the Stream trait has been added to the standard library as an unstable feature, we can start developing the next major version.

For now, I'm planning to release the next futures-core as 1.0 and the almost of other futures- * crates as 0.4, and not change the version of futures-io.

  • futures-core: 0.3 -> 1.0
  • futures-io: as-is
  • others: 0.3 -> 0.4

See milestone for changes planned or considered.

I think the release of futures 0.4/futures-core 1.0 may be the time when the stream stabilizes. (Unlike what I said in #2207.)

cc #2207

r? @cramertj @Nemo157 @seanmonstar
FYI @rust-lang/wg-async-foundations

@taiki-e
Copy link
Member Author

taiki-e commented Feb 13, 2021

This PR only contains a commit that increases the version number.
Individual changes will be separated into their own PR. e.g., #2340, #2341, #2342, #2343

@taiki-e
Copy link
Member Author

taiki-e commented Feb 13, 2021

This PR only contains a commit that increases the version number.
Individual changes will be separated into their own PR. e.g., #2340, #2341, #2342, #2343

If merging these PRs into this PR makes it easier to review, I can do it.

@yoshuawuyts
Copy link
Member

Moving to futures-core 1.0 makes sense. But I'm less sure about a 0.4 release for futures-io -- wouldn't this effectively require an upgrade of every dependent crate?

If I understand this correctly this would force us to release a new major of async-std if we choose to continue to tail the latest futures version. Which sure we can do, but given there don't appear to be any breaking changes planned for AsyncRead / AsyncWrite it makes me wonder if we could version that separately.

@seanmonstar
Copy link
Contributor

It does seem like futures-io doesn't need to change: it doesn't take a dependency on anything, and the traits aren't changing... Unless, and I don't remember the decision here, if we were to change to adopting ReadBuf for poll_read and things.

@yoshuawuyts
Copy link
Member

Unless, and I don't remember the decision here, if we were to change to adopting ReadBuf for poll_read and things.

I wouldn't go so far to say decisions have been made, but we did arrive at a concensus in #2209 that if we can keep things from breaking we should.

There may be conversations to be had on the design of the IO traits, but I think it'd be better to have those as part of moving AsyncRead / AsyncWrite to the stdlib, rather than going through yet another round of breakage in the ecosystem.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 17, 2021

I don't plan to make breaking changes on futures-io for now (see also rust-lang/wg-async#23 (comment)). Adding ReadBuf/poll_read_buf/is_{read/write}_vectored isn't a breaking change. However, if we make poll_read_buf the required method (more preferable when considering performance), it's a breaking change.

If I understand this correctly this would force us to release a new major of async-std if we choose to continue to tail the latest futures version. Which sure we can do, but given there don't appear to be any breaking changes planned for AsyncRead / AsyncWrite it makes me wonder if we could version that separately.

Even if we don't bump futures-io, many crates also depend on stream, so async-std and others need major version bump anyway.

@tmandry
Copy link
Member

tmandry commented Feb 17, 2021

Agreed with the principle of breaking as little code, and requiring as few incompatible-version migrations, as possible.

I am on board with bumping futures-util, but the name implies to me that crates won't re-export from there so the impact should be fairly limited.

Why does stream require a major version bump?

@taiki-e
Copy link
Member Author

taiki-e commented Feb 19, 2021

I am on board with bumping futures-util, but the name implies to me that crates won't re-export from there so the impact should be fairly limited.

I'm not sure what the question means, but futures and futures-util don't define their own abstractions, so if you don't implement 0.3 stream, sink, io, spawn, arcweak, and *ext traits in public APIs, updating futures/futures-util versions does not cause breaking change.

If you implement those traits, it depends on the actual version of those traits.

see also #2295

Why does stream require a major version bump?

futures-core 1.0 stream and std stream have the same trait, but futures-core 0.3 is different. Updating from 0.3 to 1.0 / std is breaking change (if you are using futures-core 0.3 stream with a public API).

see also RFC1105

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Feb 19, 2021

Stream and breaking changes

Updating from 0.3 to 1.0 / std is breaking change (if you are using futures-core 0.3 stream with a public API)

Is that true though? Much like an API moving from std to core and re-exporting it isn't a breaking change, I don't see why inherently an API moving from a third-party library to core and being re-exported would inherently be breaking?

The things we have to be concerned with is the external API promises we've made: this includes the shape of the API, the methods we implement, and the MSRV. But I don't see why re-exporting a type rather than implementing your own would inherently be breaking. Currently the API doesn't include any changes, and workarounds exist for MSRV issues.

What is the purpose of futures-core 1.0.0?

We should probably also ask ourselves which role we want futures-core 1.0.0 to play. If all it contains are re-exports from the stdlib, why would we recommend people to upgrade to it in the first place? I think the case for updating futures-core becomes much more pronounced if we can for example provide an upgrade path. We could detect the older versions of Rust and provide our own API version if we're missing any. This would allow the ecosystem to gradually transition to the stdlib types with minimal breakage.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 19, 2021

please read #2207 (comment) and #2207

@yoshuawuyts
Copy link
Member

@taiki-e I've read those comments, and I'm still missing clear motivation for why we're seeking to publish a breaking release of futures-core and futures-io. There currently don't appear to be any breaking changes planned for futures-io. And as far as I've seen there currently there are no known incompatibilities between the futures-core and the stdlib [1].

futures-rs libraries should be versioned with the goal of breaking as little of the ecosystem as possible. This means the default should be that we don't issue any breaking changes unless extremely well-motivated [2]. I'm still missing a clear articulation why we believe we have no other option but to break every piece of Rust code relying on futures-core and futures-io [3].


Quoting @cramertj's async interview who covered this topic rather well:

Presuming that some trait from futures is moved wholesale into libstd (i.e., without any modification), then it is possible for us to simply issue a new minor version of the futures crate (and the appropriate subcrate). This new minor version would change from defining a trait (say, Stream) to re-exporting the version from std.

As a concrete example, if we moved AsyncRead from futures-io to libstd (as cramertj advocates for later on), then we would issue a 0.3.2 release of futures-io. This release would replace trait AsyncRead with a pub use that re-exports AsyncRead from std. Now, any crate in the ecosystem that previously depended on 0.3.1 can be transparently upgraded to 0.3.2 (it’s a semver-compatibly change, after all)1, and suddenly all references to AsyncRead would be referencing the version from std. (This is, in fact, exactly what happened with the futures trait; in 0.3.1., it is simply re-exported from libcore.)


[1]: This may change of course if we add e.g. Stream::next. But even then I want us to cover how every option to prevent breakage in the ecosystem has proven infeasible and we had no other option but to issue a breaking change.

[2]: That could mean we don't ever publish a v1.0.0 of futures-core, and that is fine. Stdlib types effectively function as the 1.0.0 versions of the APIs under discussion. For example futures_core::FusedStream could be marked as deprecated and moved to a separate library. This by itself is not enough to warrant breaking all of the async ecosystem.

[3]: This is the kind of breakage that would warrant coverage on "inside Rust" or possibly even the main Rust blog. Given how much of Rust relies on futures-core and futures-io, any kind of breakage requires clear communication and motivation on the changes and guidance on how to migrate. Failure to do so would adversely affect Rust's reputation of being reliable and hurt adoption.

@Nemo157
Copy link
Member

Nemo157 commented Feb 19, 2021

And as far as I've seen there currently there are no known incompatibilities between the futures-core and the stdlib.

futures-core changing to re-export Stream from std is a breaking change. The following code will compile with futures-core v0.3.12 + whichever version of std first stabilizes Stream:

struct Foo;
impl std::stream::Stream for Foo { ... }
impl futures_core::stream::Stream for Foo { ... }

If futures-core v0.3.13 then detects std::stream::Stream's availability and changes to pub use std::stream::Stream; that code will no longer compile.

@Nemo157
Copy link
Member

Nemo157 commented Feb 19, 2021

I mentioned in #2207 (comment) that this could be an acceptable unchecked future-incompatibility, if we document now on futures-core::stream::Stream that it will become this re-export then users should just avoid writing code like this that will fail to compile in the future. Especially as there should be plenty of time to setup the auto-detection and re-export so that before std::stream::Stream is stabilized the latest version of futures-core should already make this code impossible.

@yoshuawuyts
Copy link
Member

I mentioned in #2207 (comment) that this could be an acceptable unchecked future-incompatibility, if we document now on futures-core::stream::Stream that it will become this re-export then users should just avoid writing code like this that will fail to compile in the future.

I'd be strongly be in favor of doing this. In addition to that we can issue guidance on the compatibility between futures-rs and std::core::Stream when we stabilize Stream. We're in the fortunate position that we can coordinate the releases of both, and can minimize accidental compat issues.

@Nemo157 would you be willing to file a PR for that?

@tesaguri
Copy link
Contributor

That could mean we don't ever publish a v1.0.0 of futures-core

Even if futures-core v0.3 decides to re-export std's Stream, futures-core can just publish a v1.0.0 release and apply the semver trick in a v0.3.x release, as long as the MSRV remains the same, so it does not need to stick to v0.3.x.

Sticking to unstable versions (v0.x) would have a subtle implication for downstream libraries which conform to C-STABLE of the Rust API Guidelines: it would mean that they can never publish a stable release in principle, so I think it should be avoided if possible.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 20, 2021

First, I agree that futures-io does not need to be breaking changed as I already mentioned. I'll revert the version bump of futures-io.

As for futures-core, I'm okay with using the tricks like that @Nemo157 and @tesaguri said. However, if the std Stream's signature becomes incompatible with futures 0.3 Stream, the 1.0 release will be breaking change and we will not introduce this trick.

...And keep in mind that it is t-libs and wg-async-foundations that decide on this, not the futures team. If t-libs and wg-async-foundations decide to stabilize Stream with a different signature than futures 0.3 Stream, futures team can't do anything about it. Actually, in the first proposal by wg-async-foundations, it was an incompatible signature.

To be clear:
I felt those who are currently working on Stream at wg-async-foundations didn't seem to consider compatibility with futures 0.3 much (sorry if that was my misunderstanding). So I thought they were likely to be changed again to have incompatible signatures.
However, @tmandry and @yoshuawuyts seem to be thinking compatibility issues seriously. (I am surprised because I previously felt they were positive about std streams having a different signature than futures 0.3.) So, hopefully, we can release futures-core 1.0 with compatibility with futures 0.3 Stream. And as @tesaguri mentioned, the 1.0 release allows more libraries to depend on futures-core on the public API.


This is, in fact, exactly what happened with the futures trait; in 0.3.1., it is simply re-exported from libcore.

I don't think this is true. futures::future::Future was already a reexport of core::future::Future at the stage of that first stable release of futures 0.3.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 21, 2021

@yoshuawuyts

I'm still missing clear motivation for why we're seeking to publish a breaking release of futures-core and futures-io.

With the release of futures-core as 1.0, many crates with major versions greater than 1 will be able to depend on futures-core in their public API. This is very important for Rust's async ecosystem.

Failure to do so would adversely affect Rust's reputation of being reliable and hurt adoption.

futures followed Semantic Versioning, API guidelines, and RFC1105 correctly and released crates as 0.x.
I know some crates with major versions greater than 1 depend on futures-core in public APIs. (async-rs/async-std#959, smol-rs/smol#228)
However, the problem is their API guideline violations and I am convinced that your claim that "futures's responsible" is wrong.

I always want to minimize ecosystem breakage as much as possible, but after reading your claim, I'm thinking of giving up my efforts on it. Taking responsibility for the stability guarantee of a crate that violates the API guidelines is not what I hoped.

Also I'm very disappointed that you used the word "Rust's reputation" to shift responsibility for the problem of "async-std violates API guidelines".

@taiki-e
Copy link
Member Author

taiki-e commented Feb 21, 2021

As for futures-core, I'm okay with using the tricks like that @Nemo157 and @tesaguri said.

Well, as for semver tricks, rand used it and caused problems many times, so I can't guarantee that it will be done even if the trait signatures are exactly the same.
If that adversely affects users of 0.3, I don't want to use it.
(I am convinced that the issue of "official crate frequently broken" is more important for "Rust's reputation" than the issue of "official crate was not responsible for downstream stability assurance that violates API guidelines".)

@taiki-e
Copy link
Member Author

taiki-e commented Feb 21, 2021

As for the migration guide: My projects (e.g., pin-project) have basically provided a clear description by changelog/release note (if you feel it is not clear, free feel to open an issue). I have no plans to write a migration guide as a complete blog like "inside Rust", but I'm planning to do the same thing I have done in my projects.

I guess your concern stems from the lack of migration guides from 0.1 to 0.3 (there were too many changes in that release that no one could accurately track), but I have some concrete plans for what to do in this release and I'm not planning a lot of significant changes like 0.1-> 0.3, so I think it's basically enough.

- futures-core: 0.3 -> 1.0
- futures-io: as-is
- others: 0.3 -> 0.4
@taiki-e
Copy link
Member Author

taiki-e commented Feb 26, 2021

I noticed that I was meeting all the requests here. As I said above, we'll need to make a decision later on whether to actually do that, but seems to be enough to merge this anyway.

@taiki-e taiki-e merged commit 9f40e6c into rust-lang:master Feb 26, 2021
@taiki-e taiki-e deleted the next-dev branch February 26, 2021 02:52
@taiki-e
Copy link
Member Author

taiki-e commented Feb 26, 2021

I opened a tracking issue on how to handle stream trait compatibility between 0.3 and 1.0: #2362

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 26, 2021

(cross-posted to #2362)

Hey all-- I've been a bit behind on catching up here. I'm reading the comments, but I have a few thoughts.

  • Given that this decision is controversial, I think we should establish who makes the final decision here in case that becomes necessary. Given that this is a Rust project, it has to be a Rust team. I would call this a joint decision of @rust-lang/libs and @rust-lang/lang, though probably more libs than lang. (Lang is involved because of how central Stream and Async I/O is.)
  • That said, I think that before those teams weigh in, it would be appropriate for @rust-lang/wg-async-foundations to prepare a summary. I believe this should take the form of a Collaborative Summary Document. I volunteer to kick that off, it'll be a good way for me to get up to speed. Part of this document is a kind of "best case" pro and con, written in terms of the values we've established, so that will help the teams to make an ultimate decision. (Of course we may also come to a recommendation over the course of working on that.)
  • Finally, I can confirm that the semver story around futures-io has absolutely been a matter of consideration. I'm sorry @taiki-e if we gave the opposite impression! For example, if you skim the minutes from the Stream design meeting, you'll see that we talked about semver and the next method there.

As a meta point, improving the coordination around this crate has been lacking and is something we should change. I would like to incorporate the overall role and vision for the futures crate into the async vision doc process. @taiki-e or others, perhaps we can sync up on Zulip about what shape that might take?

exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
- futures-core: 0.3 -> 1.0
- futures-io: as-is
- others: 0.3 -> 0.4
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.

7 participants