-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
sync: improve the docs for recv_many
#7201
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
base: master
Are you sure you want to change the base?
sync: improve the docs for recv_many
#7201
Conversation
| /// * Return when the number of received messages reaches `limit`. | ||
| /// * Return earlier when the channel is closed or no further messages area | ||
| /// available in the channel at the time. In these cases, | ||
| /// the number of received messages can be lower than `limit`. | ||
| /// |
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.
If I'm skimming this, I'm going to see "return when the number of received messages reaches limit" and I will get the wrong impression. I agree the current docs are bad, but I don't think this wording is good either.
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.
Could you please give me a hint in which way the current wording isn't up to par? Otherwise I'm not sure in which direction to go with this. 😅
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.
How about something like this?
Receives between 1 and `limit` messages into `buffer`.
This method is intended as a performance optimization for cases where
you wish to process messages in batches. The method may return less than
`limit` messages, so `recv_many` can *not* be used to receive a fixed
number of messages in one call.
This method waits until at least one message is available, and then
receives as many messages as it can into `buffer` efficiently. The
number of messages added to `buffer` is returned, and this number will
often be less than `limit` even if the channel is still open.
For non-zero values of `limit`, this method will never return `0` unless
the channel has been closed and there are no remaining messages in the
channel's queue. This indicates that no further values can ever be
received from this `Receiver`. The channel is closed when all senders
have been dropped, or when [`close`] is called.
Note that if [`close`] is called, but there are still outstanding
[`Permits`] from before it was closed, the channel is not considered
closed by `recv_many` until the permits are used or dropped.
The capacity of `buffer` is increased as needed.
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.
Yup that looks also good. 👍 Just two things:
Does this
Receives between 1 and
limitmessages intobuffer.
contradict this?
For non-zero values of
limit, this method will never return0unless the channel has been closed and there are no remaining messages in the channel's queue.
Maybe it makes sense to word that first sentence differently and say
Receives between 0 and
limitmessages intobuffer.
And my second point: I would like to be a bit more precise here.
..., and then receives as many messages as it can ...
If we explain here in which case no further messages are loaded into the buffer, we can also change this
The number of messages added to
bufferis returned, and this number will often be less thanlimiteven if the channel is still open.
to remove the "often" part as we've explained in which situation the function is returning before the limit of messages is reached. I think that would be an important addition to the docs.
My suggestion for that part would be:
This method waits until at least one message is available, and then receives as many messages as possible into
buffer(either the number of received messages reacheslimitor no more messages are available at the time). The number of messages added tobufferis returned, and this number can be less thanlimiteven if the channel is still open.
Now that I formulated my suggestions I am still wondering how my original proposal was not up to par. 😅 I feel like I've tried more to explain what happens in which situation compared to your proposal. In turn you've added the more obvious disclaimer that this function should not be expected to receive a limit amount of messages. At the end of the day I'm happy with any improvement of the docs here, but I'm still curious. 😉
Also, if we go with your proposal, please let me know if I should incorporate your suggestions (and give credit in the commit message I guess?) or if you will take over the contribution. :)
Trying to explain a bit better when `recv_many` will return and why in some cases the returned number of received messages is below the input parameter `limit`. Additionally the docs for the bounded and unbounded `recv_many` are adjusted to match.
5674da6 to
4d665e6
Compare
|
Little bump here. Anything I can do to improve the state of this PR? :) |
|
Hi @SwishSwushPow, I opened a duplicated PR (#7541) before noticing your PR, sorry for that. Are you still going to work on it? If so, I will close my duplicated PR. |
|
Thanks @ADD-SP for the heads up, that is very considerate of you. :) I'm absolutely willing to finalize this PR but I think at this point it would be @Darksonn 's turn to react to my comments to her proposal or add some more detail as to why my original proposal was missing the point (as I wrote in #7201 (comment)). 😅 So yeah I'm around. |
|
I would like to share my criteria for determining whether a document is good. When reading a crate's documentation, I often quickly read through the document in about ten seconds, then modify it based on the examples, and only read the document carefully if I have further questions. Therefore, when I write documentation or review someone else's documentation, I use this as my main criterion for evaluation. I hope this can help you and other reviewers. |
|
A good docs is not just about wording, it also includes the interface itself. For example, is the interface self-descriptive? Will the behavior surprise the downstream? Good wording + good interface design (least astonishment) allows people like me to use the interface smoothly after reading the document for only 10 seconds. Just some random, trivial thoughts. |
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'd like to move forward this PR. I personally biased towards to make it can be read in 10 seconds.
This method waits until at least one message and pushed it into the `buffer`,
then tries to extend `buffer` with up to `limit - 1` additional
messages that are immediately available in the channel queue.
However, this method doesn't push anything to the `buffer` if the `limit` is zero
or the channel is unable to yield any message.
A channel that is closed and has no active permits
is considered unable to yield messages.
This is a shorter version, which meets my biased flavor.
Trying to explain a bit better when
recv_manywill return and why in some cases the returned number of received messages is below the input parameterlimit.Motivation
I stumbled upon
recv_manyfor the first time ever in a bit of code while reviewing a PR. I took some issue with how the function was called (inside awhileloop) and had a look at the docs to understand why that would be necessary. When reading through the docs it seemed to me as if there was seemingly no need for a loop becauserecv_manywould apparently wait for new messages if none were available, only returning when thelimitwould be reached or the channel would be closed. I gave this a try in a smaller code example and had to realize that this was not the case. Looking at the source I found thatrecv_manywould only wait for the first message, but would return early if the channel queue had no messages available after that, even when not being closed. At this point I felt that the docs were a bit unclear in that regard.Solution
By describing the behavior a bit more explicitly I am hoping to improve the docs and avoid misunderstandings like mine in the future.
closes #7540