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 isfull for buffered Channels #52470

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

Conversation

tamasgal
Copy link

@tamasgal tamasgal commented Dec 10, 2023

Currently the only way to check if a Channel is full by accessing its fields .data and .sz_max which is neither a good practice, nor documented.

Checking if a (buffered) channel is full is mandatory when somebody wants to discard the data to avoid e.g. memory pile-up or redirect it to another channel (load-balancing).

This PR implements isfull(c::Channel) which returns true if the number of available items is equal (or exceeds -- still not sure if that case can happen due to race conditions) the size of the buffered channel.

A somewhat similar problem is also addressed in #41966 with the implementation of tryput!.

@jakobnissen
Copy link
Contributor

Do we have a supported way to check if a channel is buffered? Currently, isbuffered is unexported.
I think in the absence of such a function, having the API of this function here return false when the channel is unbuffered is not a good idea, because a user has no way of knowing if isfull returns true because there is still room in the channel, or because it's unbuffered.

Another issue is - suppose a user calls isfull on a buffered channel, and it returns false. Can the user then put values into it? Maybe not! If another thread has filled it immediately after the isfull call, the call will be misleading. A better way may be something similar to tryput! - the user tries to put the value in, and gets told if the channel is full, or closed, or the value was successfully put in.

@tamasgal
Copy link
Author

Not sure about the first comment. I'd say we should export isbuffered but in any case, isfull returning false for unbuffered channels is perfectly reasonable.

I guess the difference in my view comes from the fact that the main purpose to me for isfull is to be able to discard data when the channel is full. I don't think that checking isfull to decide whether to add data should be recommended at all. However, when the channel is full and the according task is not able to keep up, it doesn't really matter if a consecutive put! is blocking because the channel got full after isfull returning false and hittig the put! function.

Anyways, I am also happy with tryput!, but not sure how to proceed. #41966 implements a lot more and seems to be stuck, so either I modify this PR to implement tryput! and discard isfull or another option would be to augment put! with a keyword argument discard_when_full=true/false.

I am open for suggestions.

@ararslan ararslan added the needs news A NEWS entry is required for this change label Dec 11, 2023
@vtjnash
Copy link
Member

vtjnash commented Dec 11, 2023

For buffered channels, the equivalent measure would be how many takers are currently blocked waiting for input. The channel is full if there are more putters waiting than takers waiting (it is also how a buffered channel can exceed the queue length, since it is nbuffered+nwaiters)

Wouldn't tryput be more useful here than isfull? We could have both, but it feels already a little awkward to me that a lot of this API assumes single reader-writer (like this PR) and that could mislead users to use race-y API instead of other equivalent APIs

@vtjnash
Copy link
Member

vtjnash commented Dec 11, 2023

FWIW, #41966 is stuck because the original author is gone and it needed someone to come take over and make decisions there

@tamasgal
Copy link
Author

OK I feel it's vibing towards tryput! . Should I take over the old PR or transform this one?

@vtjnash
Copy link
Member

vtjnash commented Dec 11, 2023

I'd say make a new one, but it is fine if you want to change this one instead to include it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants