-
Notifications
You must be signed in to change notification settings - Fork 29
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
Conversation
Thoughts about the 'ideal' |
There was a problem hiding this 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.
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
*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.* |
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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
.
- **Channels:** [`async-channel`] provides an asynchronous MPMC channel, while | ||
[`oneshot`] provides an asynchronous oneshot channel. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.) |
d55908e
to
0742293
Compare
@notgull I only raised one concern, which didn't get addressed? |
0742293
to
945a72c
Compare
I missed that one @zeenix, I've added a link to the future |
945a72c
to
efa021b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
efa021b
to
d71c578
Compare
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>
d71c578
to
aa1a2fe
Compare
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 userswhich features
futures-lite
does not implement, as well as potentialcontributors which PRs will not be accepted.
cc #111
cc @smol-rs/admins Please give me feedback on this document.