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

Add list of excluded features #112

Merged
merged 1 commit into from
Nov 23, 2024
Merged

Add list of excluded features #112

merged 1 commit into from
Nov 23, 2024

Conversation

notgull
Copy link
Member

@notgull notgull commented Nov 3, 2024

This commit adds a list of features that have been determined to be out
of scope for futures-lite. The intent of this list is to inform users
which features futures-lite does not implement, as well as potential
contributors which PRs will not be accepted.

cc #111

cc @smol-rs/admins Please give me feedback on this document.

@stackinspector
Copy link

Thoughts about the 'ideal' Sink API: In my view this API is certainly ideal with cases where IO is not involved like channels. But for IO-involved cases, an error type is reasonable, and it can be considered to take the object to send as a reference. So should it be 2 APIs?

Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 😎 LGTM otherwise but I am not sure a file in the repo will have sufficient visibility. I think most (all?) of this should be part of the crate docs or at least linked from there.

FEATURES.md Outdated Show resolved Hide resolved
FEATURES.md Outdated Show resolved Hide resolved
represent a significant uptick in complexity and thus is better suited to
other crates.

[`futures-concurrency`] provides a number of simple APIs for dealing with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to officially recommend this crate because there was a lot of suspicious unsafe code when I looked at it before.
rust-lang/futures-rs#2851 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @yoshuawuyts

I think it's better now, but I'm not sure, I haven't fully reviewed the code.

Comment on lines +231 to +227
*Sidenote: [`Stream`], [`AsyncRead`] and [`AsyncWrite`] suffer from this same
problem to an extent. I think they could also be fixed by transforming their
`fn poll_[X]` functions into `async fn [X]` functions. However their APIs are
not broken to the point that [`Sink`]'s is.*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't think async fn in trait is the right solution (e.g., see rust-lang/rfcs#3710 (comment)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I disagree. In fact async-io basically supports the "N readers N writers" use case without much needed changes. Not to mention AsyncRead and AsyncWrite in their current state are problematic because they don't support completion based APIs like io_uring.

FEATURES.md Outdated Show resolved Hide resolved
FEATURES.md Outdated Show resolved Hide resolved
Comment on lines +248 to +242
- **Channels:** [`async-channel`] provides an asynchronous MPMC channel, while
[`oneshot`] provides an asynchronous oneshot channel.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async-channel has an optimization to handle single capacity case well, and I think it is also okay to use it for oneshot case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time I've heard of it! I'll give it a try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that async-channel requires an additional allocation for oneshot channels over the [oneshot] crate.

@taiki-e
Copy link
Collaborator

taiki-e commented Nov 3, 2024

As for alternatives to Sink, there was some discussion about something like AsyncExtend (async-std has it but inefficient since that requires boxing).

(Personally, I have the impression that Sink has (potential) users with a variety of different requirements, that there is not yet a solution that sufficiently meets them, and that abstraction is often not actually needed there in the first place.)

FEATURES.md Outdated Show resolved Hide resolved
FEATURES.md Outdated Show resolved Hide resolved
FEATURES.md Outdated Show resolved Hide resolved
@notgull notgull force-pushed the notgull/list branch 2 times, most recently from d55908e to 0742293 Compare November 8, 2024 04:44
@notgull notgull requested review from taiki-e, fogti and zeenix November 9, 2024 03:54
@zeenix
Copy link
Member

zeenix commented Nov 11, 2024

@notgull I only raised one concern, which didn't get addressed?

@notgull
Copy link
Member Author

notgull commented Nov 12, 2024

I missed that one @zeenix, I've added a link to the future FEATURES.md to the docs for futures-lite.

Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/lib.rs Show resolved Hide resolved
This commit adds a list of features that have been determined to be out
of scope for `futures-lite`. The intent of this list is to inform users
which features `futures-lite` does not implement, as well as potential
contributors which PRs will not be accepted.

cc #111

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull merged commit d5d1b19 into master Nov 23, 2024
7 checks passed
@notgull notgull deleted the notgull/list branch November 23, 2024 19:13
@notgull notgull mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants