-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Update mpsc capacity docs #7703
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
Open
smklein
wants to merge
4
commits into
tokio-rs:master
Choose a base branch
from
smklein:capacity-docs
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+28
−12
Open
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think this wording is too pedantic, and adding this would be confusing. If you receive a value and there is a call to
reserve, then from the caller's perspective it just went up and then immediately went down again. The fact that we are able to avoid the up-then-down is an implementation detail.The primary purpose of this section is to explain how
capacity()andlen()are different.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 agree that the wording is a bit pedantic. To back up a second, I'm trying to cope with the following case:
We've hit this issue in our codebase (oxidecomputer/omicron#9272).
This deadlock makes sense to me with the knowledge that:
tokio::mpsc::Senderuses a semaphore queue for FIFO access to capacity, and callers are enqueued for access to this semaphore when they callsendorreserveand get backPoll::Pending.tokio::mpsc::Receivercallsrecv, if there are any pending waiters, it grants capacity directly to pendingtokio::mpsc::Sender::sendfutures, if any exist, regardless of whether or not they are being polled. Granting capacity is distinct from granting access to a particular slot.There is a comment on
sendandreserveabout this:But I think there's important nuance in my aforementioned case: when we say "calls to send and reserve complete in the order they were requested", we're not saying "they will return
Poll::Ok" in a particular order, nor are we saying "we will enforce that they send messages in the mpsc with an ordering of when they were first polled". Rather, this statement actually means:sendandreceivein this FIFO order (internally: permits from the semaphore queue) based on their calling ordersendto get a permit, but yet not be re-polled for a while - it would not necessarily block the order of the mpsc by taking a specific slot, but it would be holding on to a portion of the total capacity.I created a playground link for this case here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c4f2c5b7b0616eee84eee4ba63193fdc
I'm struggling with treating this as an implementation detail - "how capacity works", along with the doc comment about "calls to send and reserve complete in the order they were requested", leave a few user-visible oddities:
recvcan cause the capacity to go from zero -> zero, even with no concurrent operations, just with the existence of a prior incompletesend/reservefuture in existence somewhere. This feels at odds with the comment that "The capacity goes up when values are received by the Receiver".send/reserveand getting back aPoll::Pendingfuture has a lot of implications for ordering, which are also user-visible: as long as the future remains non-cancelled, the guarantee provided here is that capacity is granted to the futures in FIFO order, not that the calls tosend/reservewill actually complete in a particular order.Both of these aspects combined led me to "wanting to clarify what capacity means, and how it is provided to callers, especially in the context of a saturated channel".
Uh oh!
There was an error while loading. Please reload this page.
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.
At the risk of being even more pedantic (sorry), I don't agree that there are "no concurrent operations" in this case; I think "the existence of a prior incomplete
send/reservefuture" is reasonably considered a concurrent operation, even if it is not executing in parallel. When multiple futures are multiplexed into a single task using concurrency primitives likeselect!,FuturesUnordered,join!, et cetera, we generally think of this as a form of concurrency even if it is not parallelism.I do, however, agree with @smklein that we should make sure it's clear to the user of the MPSC that capacity is always assigned to outstanding concurrent send operations in the order that they started to wait on
send/reservecalls, even if thosesend/reservecalls do not complete in that order. And, we should make sure that it is clear to the caller that in situations where there are outstandingsend/reservecalls, the capacity may not be visible in calls tocapacity()if it has already been assigned to a waiting sender.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.
On the concurrency note - valid point, especially with the example above using a timer. That's definitely concurrent, but not parallel. But I thiiiiiiink the playground example (https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c4f2c5b7b0616eee84eee4ba63193fdc) is not concurrent, even though there is a biased
select!operation in-play. I'm just using it as a shorthand for:Which I expect it'll do in a deterministic ordering, because of the
biasedkeyword.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'm happy to see the things you mentioned be documented. But I don't think the
fn capacity()docs are the right place. Perhaps you could add a new section to the struct docs instead?