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 {Sender, Receiver}::is_connected #497

Closed
wants to merge 1 commit into from
Closed

Add {Sender, Receiver}::is_connected #497

wants to merge 1 commit into from

Conversation

Thomasdezeeuw
Copy link
Contributor

Checks if the other side of the channel is connected.

Updates #256 (comment)

@Thomasdezeeuw
Copy link
Contributor Author

The CI failure seems unrelated.

@BurntSushi
Copy link

What is the use case for methods like this? They seem inherently problematic to use. At the very least, the docs on these methods should discuss this.

@Thomasdezeeuw
Copy link
Contributor Author

The use case is I want to know before creating a message if the receiving end is still connected, if not I can avoid doing any work for a message that is never going to be received.

They seem inherently problematic to use.

What do you see as problematic? Of course the method is inherently race-y as the moment the method returns true it could already be false, but its still an optimisation I consider useful.

@BurntSushi
Copy link

Its raciness is exactly why I consider it problematic. I don't maintain this library, but I personally wouldn't accept a new API item like this without a reasonable benchmark. If we must have them, the docs should do their best to de-emphasize using this routine and explain their niche use case.

@Thomasdezeeuw
Copy link
Contributor Author

What do you want to benchmark? If creating a message is expansive, then its always going to be faster to not create a message than creating it and the dropping it (after failing to send it).

@BurntSushi
Copy link

That sounds like a micro-benchmark to me. I'd prefer seeing something more holistic.

@Thomasdezeeuw
Copy link
Contributor Author

Sorry @BurntSushi but I have no idea what you want from me... I asked what you want me to benchmark but you haven't really given an answer to that.

@BurntSushi
Copy link

@Thomasdezeeuw I'm not the maintainer of this library. I'm just saying what I personally would like to see because I personally find these APIs to be of somewhat dubious value. Which in turn, to me, means they should be really well motivated before adding them. Namely, I'd like to see a holistic example of a real use case where these sorts of APIs would help. I would probably expect some kind of latency/throughput figures with/without these APIs for example. It may be hard to provide this.

@Thomasdezeeuw
Copy link
Contributor Author

@stjepang could you take a look at this? In #256 (comment) you mentioned this feature.

@taiki-e
Copy link
Member

taiki-e commented Oct 20, 2020

I don't have a very strong opinion on whether to add this API at this time, but I think it's better to be is_disconnected instead of is_connected. (It matches existing APIs such as error type.)

@Thomasdezeeuw
Copy link
Contributor Author

@taiki-e I'm ok with changing it, but would that mean this would be considered to be merged?

@taiki-e
Copy link
Member

taiki-e commented Oct 27, 2020

@Thomasdezeeuw I think documentation about what users should be notes of when using this API is needed to merge this PR, not only renaming. FYI: golang/go#20680 and golang/go#30906 have some specific explanations about the problems of similar features.

@Thomasdezeeuw
Copy link
Contributor Author

This has taken too long and I've moved on to using a different crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants