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

sync: Update broadcast::recv to return a named future #6908

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Oct 17, 2024

Motivation

Closes #6902.

Solution

I only made the minimal change of adding the public Recv future type.

I also briefly looked into updating tokio_stream::wrappers::BroadcastStream to not use ReusableBoxFuture, but it's not as easy as I had hoped as it owns the underlying receiver while borrowing it when being polled - i.e. it's self-referential which is only possible with async fn or some extra unsafe code.

For my main use case for this though (in my own crate), the unsafe code + impl !Unpin is likely preferable over the hack I shared in the issue linked above. I don't know whether the same applies to tokio-stream.

@jplatte jplatte added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Oct 17, 2024
@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Oct 17, 2024
@Darksonn
Copy link
Contributor

Are you sure this actually allows you to solve #6902? The stream is still going to need to be self-referential.

@jplatte
Copy link
Member Author

jplatte commented Oct 18, 2024

Well, I think it helps. My current plan for my downstream crate is to write a self-referential RecvOwned based on this by hand with unsafe - that seems less awkward to me than the previous workaround. Would that be something you would accept into tokio-stream as well (I guess if it was there, I could depend on tokio-stream in my downstream crate too)?

I can also think of two alternatives:

  • Introduce NonSendReusableBoxFuture in tokio-util (the unsafe c'tor for ReusableBoxFuture seems much more risky to me, I tried something), use the same workaround as now but without copy-pasted code downstream
  • Not sure whether it's possible, but I think it would be: Add Receiver::recv_owned(self) -> RecvOwned<T>, impl<T: Clone> Future for RecvOwned<T> { type Output = (T, Receiver<T>); } and avoid the whole business of self-referential futures

@Darksonn
Copy link
Contributor

Darksonn commented Oct 19, 2024

I don't love exposing future types because it makes the documentation worse. Right now, the function is documented as an async fn, but it's going to look like a synchronous function with this change. That's not to say we absolutely can't do it, and we have done so in the past with tokio::sync::futures::Notified. But I think it is unfortunate.

By the way, Notified is used for something similar here:

#[must_use = "futures do nothing unless polled"]
pub struct WaitForCancellationFutureOwned {
// This field internally has a reference to the cancellation token, but camouflages
// the relationship with `'static`. To avoid Undefined Behavior, we must ensure
// that the reference is only used while the cancellation token is still alive. To
// do that, we ensure that the future is the first field, so that it is dropped
// before the cancellation token.
//
// We use `MaybeDanglingFuture` here because without it, the compiler could assert
// the reference inside `future` to be valid even after the destructor of that
// field runs. (Specifically, when the `WaitForCancellationFutureOwned` is passed
// as an argument to a function, the reference can be asserted to be valid for the
// rest of that function.) To avoid that, we use `MaybeDangling` which tells the
// compiler that the reference stored inside it might not be valid.
//
// See <https://users.rust-lang.org/t/unsafe-code-review-semi-owning-weak-rwlock-t-guard/95706>
// for more info.
#[pin]
future: MaybeDangling<tokio::sync::futures::Notified<'static>>,
cancellation_token: CancellationToken,

(The ci failure is fixed on master.)

@jplatte
Copy link
Member Author

jplatte commented Oct 19, 2024

Right, I agree about async fn looking better in the docs.

So, what would be your preferred solution here? I can go with whatever, including no changes to tokio at all (I have a working solution downstream already afterall, even though the reasoning around it being safe is a bit thin).

@Darksonn
Copy link
Contributor

We can add this. Please expose the future in tokio::sync::futures and add a note to the docs about what the async fn signature would be, similar to how the Ext traits do so.

@jplatte
Copy link
Member Author

jplatte commented Nov 6, 2024

I have implemented the changes you asked for.

I haven't had any focus time for finishing the hand-written self-referential future based on this (what motivated me to write this PR). One thing I have already found is that the T: Clone bound of the Recv type (rather than only its Future impl) might get in the way a little bit, but that could be relaxed later – it's currently required due to Coop, but I'm pretty sure could be removed by doing the Coop logic with a freestanding fn rather than wrapping RecvInner in Coop.

Given this uncertainty, I wouldn't mind if merging this PR was delayed until I finish the downstream work that needs this, though it could lead to bitrot. However you want to deal with this is fine by me.

@Darksonn
Copy link
Contributor

Darksonn commented Nov 8, 2024

Inlining the coop logic into RecvInner::poll is fine with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom-sync Run loom sync tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Allow non-Send broadcast values in more (generic) contexts
2 participants