sync: fix panic in Chan::recv_many when called with non-empty vector on closed channel#7991
Conversation
Darksonn
left a comment
There was a problem hiding this comment.
Thanks!
Merge without squash is recommended.
If you add the test first, then I will need to merge with squash. Otherwise bisection will not work on the main branch if someone hits the first commit in a bisection.
| // Does not panic, no longer expecting an empty buffer (#7990). | ||
| rx.recv_many(&mut buffer, 1).await; |
There was a problem hiding this comment.
I think that if someone sees this comment in one year, they will be confused about what you mean by "no longer". They don't know the context of this bug. Instead of this comment, can you add a comment above this test saying that it is a regression test for <link to issue>?
There was a problem hiding this comment.
If you add the test first, then I will need to merge with squash. Otherwise bisection will not work on the main branch if someone hits the first commit in a bisection.
Both commits are squashed and the commit message and PR description updated.
I think that if someone sees this comment in one year, they will be confused about what you mean by "no longer". They don't know the context of this bug. Instead of this comment, can you add a comment above this test saying that it is a regression test for
<link to issue>?
Following up on #7991 (comment), I have extended the scope of the tests from panic-regression to behavior test. Therefore, the suggested comment seems no longer useful. The original comment is removed.
|
|
||
| if rx_fields.rx_closed && self.inner.semaphore.is_idle() { | ||
| assert!(buffer.is_empty()); | ||
| assert!(buffer.len() == initial_length); |
There was a problem hiding this comment.
| assert!(buffer.len() == initial_length); | |
| assert_eq!(buffer.len(), initial_length); |
There was a problem hiding this comment.
I chose assert! because it is a compiler intrinsic while assert_eq! is much more involved.
There was a problem hiding this comment.
Using assert_eq! is better because it prints the values when they are not equal. But if you have perf concerns, we can make it into a debug_assert_eq!.
There was a problem hiding this comment.
Thanks. I would leave the decision up to you.
Performance is not a huge concern, as this is not a hot path, but of course the panic unwinding machinery will be part of the generated assembly.
Is seems useful to keep consistent with https://github.com/LeoniePhiline/tokio/blob/2222621764ca3b6fd70803874f76ccdc59885ca7/tokio/src/sync/mpsc/chan.rs#L389 and https://github.com/LeoniePhiline/tokio/blob/2222621764ca3b6fd70803874f76ccdc59885ca7/tokio/src/sync/mpsc/chan.rs#L315 at the other exit of the pop loop – should these be changed to debug_assert?
I myself would prefer debug assertions, as these are checking for bugs, not UB. And, to my mind, they are meant to verify the implementation in tests, not production code.
2222621 to
7dd98a3
Compare
| // TODO: This check may not be required as it most | ||
| // likely can only return `true` at this point. A | ||
| // channel is closed when all tx handles are | ||
| // A channel is closed when all tx handles are | ||
| // dropped. Dropping a tx handle releases memory, | ||
| // which ensures that if dropping the tx handle is | ||
| // visible, then all messages sent are also visible. | ||
| assert!(self.inner.semaphore.is_idle()); | ||
| debug_assert!(self.inner.semaphore.is_idle()); |
There was a problem hiding this comment.
Following up with #7991 (comment), and after my review of the internals, I am proposing to demote these bug checks (not UB guards) to cfg(debug).
This aligns these pop loop exit assertions with the rx_closed exit assertion.
There was a problem hiding this comment.
IMO such changes should happen in separate PR.
|
I'm going to backport this to Tokio 1.47, so I'm changing the base of this PR now. |
7dd98a3 to
41a87ad
Compare
Chan::recv_many must not make assumptions about buffer contentsChan::recv_many when called with non-empty vector on closed channel
…r on closed channel (tokio-rs#7991) `Chan::recv_many` intends to assert that no slots have been consumed when exiting with `Ready` via the `rx_closed` code path. Instead of asserting no items were added to the buffer, it asserted buffer emptiness, incorrectly making assumptions about the provided buffer. When `recv_many` was called on an empty channel with idle semaphore after the receiver was closed, the method would panic. The branch coverage had been previously missing. This changeset corrects the assertion and adds tests covering the code path. Fixes tokio-rs#7990.
41a87ad to
351c4f5
Compare
…r on closed channel (tokio-rs#7991) `Chan::recv_many` intends to assert that no slots have been consumed when exiting with `Ready` via the `rx_closed` code path. Instead of asserting no items were added to the buffer, it asserted buffer emptiness, incorrectly making assumptions about the provided buffer. When `recv_many` was called on an empty channel with idle semaphore after the receiver was closed, the method would panic. The branch coverage had been previously missing. This changeset corrects the assertion and adds tests covering the code path. Fixes tokio-rs#7990.
351c4f5 to
bf18ed4
Compare
|
Thank you! |
Motivation
Chan::recv_manyintends to assert that no slotshave been consumed when exiting with
Readyviathe
rx_closedcode path.Instead of asserting no items were added to the
buffer, it asserted buffer emptiness, incorrectly
making assumptions about the provided buffer.
When
recv_manywas called on an empty channelwith idle semaphore after the receiver was closed,
the method would panic.
Solution
The branch coverage had been previously missing.
This changeset corrects the assertion
and adds tests covering the code path.
Fixes #7990.