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

Item type of broadcast Stream #3214

Closed
Darksonn opened this issue Dec 4, 2020 · 8 comments
Closed

Item type of broadcast Stream #3214

Darksonn opened this issue Dec 4, 2020 · 8 comments
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Dec 4, 2020

The item type of the broadcast into_stream Stream is Result<T, RecvError>, but that RecvError has a variant called Closed. We should reconsider this, as the stream should just return None in that situation.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Dec 4, 2020
@Darksonn Darksonn added this to the v1.0 milestone Dec 4, 2020
@carllerche
Copy link
Member

You are correct in that the stream should represent "closed" as returning None. Are you proposing defining a new error type for the stream version that does not have Closed?

@Darksonn
Copy link
Contributor Author

Darksonn commented Dec 4, 2020

That would certainly be an option. I'm also open to other solutions, but I'm not sure what alternatives there are.

@carllerche
Copy link
Member

@Darksonn How do you think we should proceed here?

@Darksonn
Copy link
Contributor Author

Darksonn commented Dec 10, 2020

I think we should add a new error type. Considering we already have an entire module for errors for broadcast, I do not think that is an issue API-complexity wise, as people probably wont look into that module very much.

I also think it would make sense to make the impl Stream return type by into_stream a concrete type called e.g. ReceiverStream. This one in particular is often used together with things like StreamMap or SelectAll and then put in a struct, so I've seen people want to name the type several times.

@LucioFranco
Copy link
Member

We are removing the stream impl in #2870, so I think we can punt on this for now?

@Darksonn
Copy link
Contributor Author

Fair point.

@LucioFranco LucioFranco removed this from the v1.0 milestone Dec 11, 2020
@henryboisdequin
Copy link

Is this still relevant and an issue to work on?

@Darksonn
Copy link
Contributor Author

Ah, thanks for reminding me of the issue. No, a new Stream impl was added as BroadcastStream, which does indeed use a custom enum.

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
Projects
None yet
Development

No branches or pull requests

4 participants