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

Make alloc optional (embedded rust) #479

Open
Tracked by #758
xoac opened this issue Feb 10, 2021 · 24 comments
Open
Tracked by #758

Make alloc optional (embedded rust) #479

xoac opened this issue Feb 10, 2021 · 24 comments

Comments

@xoac
Copy link

xoac commented Feb 10, 2021

Proposition:
I wonder if is it possible to make alloc optional. As far as I have checked the source code the main problem for make alloc optional is with implementation of Bytes and BytesMut that require alloc to work. To support partially bytes for embedded devices where alloc is not an option theses structs need re-implementation and this is complicated subject. Things that can be introduced to embedded word are Buf and BufMut traits and this works nearly out of the box.

Motivation:

  • common interface for bytes on every level of rust

Here is example where alloc crate is a problem:


Crate that could benefits from proposed changes:

@taiki-e
Copy link
Member

taiki-e commented Feb 10, 2021

As said in #461 (comment), I don't think it's possible without breaking changes. See also crossbeam-rs/crossbeam#508 (comment).

@xoac
Copy link
Author

xoac commented Feb 11, 2021

So now the question is a little bit political. Are authors interested to make bytes crate embedded devices friendly? Since this issue was know before first major release maybe "not make alloc optional" was considered decision?

I think breaking changes are not a problem here, since you can just simply increment major release.

@taiki-e
Copy link
Member

taiki-e commented Feb 12, 2021

The main problem is that we recently released v1 and don't want to release v2 right away.

Since this issue was know before first major release maybe "not make alloc optional" was considered decision?

I don't think there was such a decision. I knew about such an issue, but I wasn't noticed that bytes had a similar issue until #461 reported.

To be clear: I'm basically in favor of doing this in the next major version.

@Kixunil
Copy link

Kixunil commented Jan 12, 2022

There is a way to achieve embedded compatibility (and I hope MSRV 1.36) without breaking bytes API: jut move the traits only to a separate bytes-traits crate and reexport/implement them here. The embedded consumers can simply depend on that crate instead of bytes. I'm willing to make a PR for this.

@taiki-e
Copy link
Member

taiki-e commented Jan 12, 2022

There is a way to achieve embedded compatibility (and I hope MSRV 1.36) without breaking bytes API: jut move the traits only to a separate bytes-traits crate and reexport/implement them here. The embedded consumers can simply depend on that crate instead of bytes. I'm willing to make a PR for this.

Note that in this approach, it will break if the user runs cargo update after implementing the traits provided by the old version of bytes crate and the traits provided by bytes-traits crate.

@Kixunil
Copy link

Kixunil commented Jan 12, 2022

@taiki-e isn't it the same problem as having two different versions of a crate?

@taiki-e
Copy link
Member

taiki-e commented Jan 12, 2022

@Kixunil "the old version of bytes crate" meant the 1.x.y version that does not contain the changes you suggested.

They are different traits in the 1.x.y version that does not contain the changes you suggested, but they are the same trait in the version that contains the changes.

@Kixunil
Copy link

Kixunil commented Jan 12, 2022

I don't think it's a problem though. Either you have two version of the bytes crate but the same problem would happen without reexport or you have a single version and then it's non-issue.

I think you're describing this situation, please LMK if you mean something else
assuming bytes_old -> bytes_new is a minor or patch release, bytes_new moves the trait to a separate crate

  • A depends on bytes_old

  • B depends on bytes_old

  • C depends on A and B, supplying A with types from B

  • B changes to implement the traits from bytes-traits and stops depending on bytes

  • C updates the version of B without updating bytes_old

C no longer compiles

In this case B is the one doing breaking change, not bytes.
B can also trivially improve it using the semver trick:

  • B_non_breaking depends on B_breaking AND bytes_new
  • B_breaking implements the change

@nils-van-zuijlen
Copy link

Wouldn't it be possible to have alloc as a default feature flag the way that serde does it?

As I see it, the only people that would be affected by that change would be the no_std group which have set the default-features = false, and not the wider audience that uses std via the default features.

@taiki-e
Copy link
Member

taiki-e commented Jan 2, 2024

@nils-van-zuijlen Please read previous comments (#479 (comment) and #479 (comment)).

@Kixunil
Copy link

Kixunil commented Jan 3, 2024

@taiki-e I think I addressed that in my comment. Is there a specific scenario I'm missing?

@Altair-Bueno
Copy link

4 years latter with 2024 edition coming soon seems like a good time to release a breaking version that includes all of those nice to have features (separate core crate, alloc, fixes for thumb arm...)

@Darksonn
Copy link
Contributor

We have no plans to ever make a breaking change of the bytes crate.

@Altair-Bueno
Copy link

Altair-Bueno commented Jan 20, 2025

So the plan is just to pile up issues until eventually someone decides to fork the repo? It doesn't seem like a great roadmap for the crate nor gives tokio a good look. Specially when other projects, like axum, are constantly bringing in breaking changes. Its beyond my comprehension why would tokio keep such basic functionality missing. Specially when the breaking changes should affect only a small userbase.

@taiki-e
Copy link
Member

taiki-e commented Jan 20, 2025

Specially when the breaking changes should affect only a small userbase.

bytes 2.0 means tokio 2.0, this affects large userbase.

Also, some of the things you mentioned require breaking changes and some do not, and you seem to be confusing the two.
If you want something specific, I would suggest you discuss the individual cases specifically (after reading the existing discussion).

@Altair-Bueno
Copy link

Specially when the breaking changes should affect only a small userbase.

bytes 2.0 means tokio 2.0, this affects large userbase.

As far as i know, tokio requires std. Thus, no tokio user would be affected by an optional feature

If you want something specific, I would suggest you discuss the individual cases specifically (after reading the existing discussion).

This issue, for example.

@taiki-e
Copy link
Member

taiki-e commented Jan 20, 2025

bytes 2.0 means tokio 2.0, this affects large userbase.

As far as i know, tokio requires std. Thus, no tokio user would be affected by an optional feature

A breaking change that does not affect tokio is also a breaking change. And if we do that, we need to increase the major version of bytes.

For example, if this issue were done as a non-breaking change, it would break the prost user's code since prost disables default features of bytes and generates code that uses Bytes.

@Darksonn
Copy link
Contributor

Darksonn commented Jan 20, 2025

Tokio uses the bytes crate in the public API in several different places related to the AsyncRead/AsyncWrite traits for the Buf trait, so Tokio cannot upgrade from bytes v1 to a hypothetical bytes v2.

So the plan is just to pile up issues until eventually someone decides to fork the repo?

The plan is to evolve the bytes crate in a non-breaking way. That is not different from the Tokio crate. Some features will be out of scope for Tokio, and some will be out of scope for the bytes crate. That's just how it is.

As for this particular issue, we will discuss options in the Tokio Discord. There are possibilities, but none of them are great. We may decide to close this issue as out of scope.

@Kixunil
Copy link

Kixunil commented Jan 21, 2025

Then it looks like putting the traits in a separate crate is the only solution.

@taiki-e
Copy link
Member

taiki-e commented Jan 21, 2025

As I said in the Tokio Discord, (IIUC) separate crate approach is not possible without a breaking change.

Buf::copy_to_bytes returns Bytes, so (IIUC) I don't believe option 2 is actually possible without a breaking change. https://docs.rs/bytes/latest/bytes/buf/trait.Buf.html#method.copy_to_bytes

@Kixunil
Copy link

Kixunil commented Jan 21, 2025

Oh! That's true. But now I'm thinking, if it's just about features shouldn't semver trick work just fine? Release bytes 2.0 with new features, then make 1.0 depend on bytes 2.0 forcing the alloc feature to be turned on. tokio can keep depending on bytes 1.0 indefinitely but embedded folks can use bytes 2.0 to get no-alloc.

@taiki-e
Copy link
Member

taiki-e commented Jan 21, 2025

I don't think semver trick can avoid a breaking change either since the type returned by Buf::copy_to_bytes changes from bytes v1's Bytes to bytes v2's Bytes. EDIT: this is incorrect as pointed out in #479 (comment)

@Kixunil
Copy link

Kixunil commented Jan 21, 2025

The bytes 1.x would contain literally just pub use bytes_2::*; so the types would be the same. Buf::copy_to_bytes would be #[cfg(alloc)].

@Darksonn
Copy link
Contributor

Darksonn commented Jan 24, 2025

As per discussion on #tokio-internals, we have decided to go ahead with a v2 release of bytes using the semver hack.

The accepted changes for this issue involve:

  • Introduce a new feature called alloc.
  • The Bytes and BytesMut types are gated behind the alloc feature gate.
  • The implementations of Buf/BufMut for Box,Vec,VecDeque are gated behind the alloc feature gate.
  • The Buf::copy_to_bytes method will be gated behind the alloc feature gate.

To allow Tokio to upgrade from bytes v1 in a non-breaking manner, there will be a v1 release of bytes that depends on bytes v2 with the alloc feature unconditionally enabled.

Let me know if I have missed anything.

Please see #758 for more information or to suggest additional breaking changes.

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

6 participants