Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions tokio/src/sync/mpsc/chan.rs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update documentation for TryPopResult::Empty in tokio/src/sync/mpsc/list.rs to mention that it might be closed.

In fact, maybe Empty should be renamed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe Closed should be renamed to ClosedBySender?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a docs-only update would be the best approach for now:

  • Document that TryPopResult::Empty can be returned after Rx::close() when the queue is empty (even if senders still exist)
  • Clarify in the docs that TryPopResult::Closed specifically means the send half closed (all senders dropped, no further items will be produced)

I'd prefer to defer renaming the variants in this PR. My concern is that if we rename now and discover additional edge cases later, we might need to rename again, which could cause unnecessary churn.

That said, I'm open to including the rename if you feel strongly about it—just wanted to share my reasoning. Would this documentation approach work for you?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,10 @@ impl<T, S: Semaphore> Rx<T, S> {
return Ok(value);
}
TryPopResult::Closed => return Err(TryRecvError::Disconnected),
// If close() was called, an empty queue should report Disconnected.
TryPopResult::Empty if rx_fields.rx_closed => {
return Err(TryRecvError::Disconnected)
}
TryPopResult::Empty => return Err(TryRecvError::Empty),
TryPopResult::Busy => {} // fall through
}
Expand Down
6 changes: 6 additions & 0 deletions tokio/src/sync/mpsc/list.rs
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs updated per discussion at this line. If you prefer different wording, I can adjust anytime.

Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,14 @@ pub(crate) enum TryPopResult<T> {
/// Successfully popped a value.
Ok(T),
/// The channel is empty.
///
/// Note that `list.rs` only tracks the close state set by senders. If the
/// channel is closed by `Rx::close()`, then `TryPopResult::Empty` is still
/// returned, and the close state needs to be handled by `chan.rs`.
Empty,
/// The channel is empty and closed.
///
/// Returned when the send half is closed (all senders dropped).
Closed,
/// The channel is not empty, but the first value is being written.
Busy,
Expand Down
9 changes: 9 additions & 0 deletions tokio/tests/sync_mpsc.rs
Comment thread
Darksonn marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,15 @@ fn try_recv_unbounded() {
}
}

#[test]
fn try_recv_after_receiver_close() {
let (_tx, mut rx) = mpsc::channel::<()>(5);

assert_eq!(Err(TryRecvError::Empty), rx.try_recv());
rx.close();
assert_eq!(Err(TryRecvError::Disconnected), rx.try_recv());
}

#[test]
fn try_recv_close_while_empty_bounded() {
let (tx, mut rx) = mpsc::channel::<()>(5);
Expand Down