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

MPSC Sender channel not dropping buffer when all Receivers are dropped during a thread panic unwind #107466

Closed
philipr-za opened this issue Jan 30, 2023 · 2 comments · Fixed by #108164
Labels
C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@philipr-za
Copy link

Code

I tried this code:

use std::sync::mpsc;
use std::thread;
use std::time::{Duration, Instant};

struct Foo {
    data: i32,
    start: Instant,
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!(
            "Dropping Foo with data {} - {:?}",
            self.data,
            self.start.elapsed()
        );
    }
}

fn main() {
    let start = Instant::now();
    let (tx1, rx1) = mpsc::sync_channel(1);

    let _ = thread::spawn(move || {
        // Move the receiver into this thread.
        let _rx = rx1;
        thread::sleep(Duration::from_secs(1));
        panic!("expected panic");
        // Under 1.66.1 sender `tx1` and the item in it's buffer are dropped after this panic causes
        // the thread to unwind and the Receiver to be dropped
        // Under 1.67.0 the sender `tx1` and the data in it's buffer are not dropped here
    });

    tx1.send(Foo { data: 1, start }).unwrap();
    println!("Sends completed - {:?}", start.elapsed());

    thread::sleep(Duration::from_secs(5));
    println!("Ending - {:?}", start.elapsed());
    // Under 1.67.0 the sender `tx1` and the data in it's buffer are only dropped when the
    // whole process ends here
}

I expected to see this happen: I expected to see the struct Foo in the Sender buffer get dropped after the Receiver is dropped during the unwind of the panicking thread.

Instead, this happened: The data in the Sender buffer is not dropped until the whole process ends.

Version it worked on

It most recently worked on: 1.66.1

Version with regression

rustc --version --verbose:

rustc 1.67.0 (fc594f156 2023-01-24)
binary: rustc
commit-hash: fc594f15669680fa70d255faec3ca3fb507c3405
commit-date: 2023-01-24
host: x86_64-unknown-linux-gnu
release: 1.67.0
LLVM version: 15.0.6

@philipr-za philipr-za added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jan 30, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 30, 2023
@apiraino
Copy link
Contributor

apiraino commented Feb 2, 2023

bisection seems to point to afd7977 cc @Amanieu

searched nightlies: from nightly-2022-11-01 to nightly-2023-02-01
regressed nightly: nightly-2022-11-14
searched commit range: 6284998...e631891
regressed commit: afd7977

bisected with cargo-bisect-rustc v0.6.5

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2022-11-01 --end=2023-02-01 --script ./script.sh 

@apiraino apiraino added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 2, 2023
@taiki-e
Copy link
Member

taiki-e commented Feb 2, 2023

In crossbeam, the unbounded channel does this (crossbeam-rs/crossbeam#669), but the bounded channel does not. There is no particular intent, it just hasn't been implemented.

cc @ibraheemdev

@m-ou-se m-ou-se added the P-low Low priority label Feb 8, 2023
joboet added a commit to joboet/rust that referenced this issue Feb 17, 2023
Tests that messages are immediately dropped once the last receiver is destroyed.
@Amanieu Amanieu added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Mar 1, 2023
bors bot added a commit to crossbeam-rs/crossbeam that referenced this issue Mar 11, 2023
968: channel: Extend panic_on_drop test r=taiki-e a=taiki-e

Include rust-lang/rust#107466 's case.
cc rust-lang/rust#108164


Co-authored-by: Taiki Endo <te316e89@gmail.com>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 20, 2023
…, r=Amanieu

Drop all messages in bounded channel when destroying the last receiver

Fixes rust-lang#107466 by splitting the `disconnect` function for receivers/transmitters and dropping all messages in `disconnect_receivers` like the unbounded channel does. Since all receivers must be dropped before the channel is, the messages will already be discarded at that point, so the `Drop` implementation for the channel can be removed.

`@rustbot` label +T-libs +A-concurrency
@bors bors closed this as completed in 93a82a4 Mar 21, 2023
gz added a commit to feldera/feldera that referenced this issue Jun 23, 2024
The code is identical except that
rust-lang/rust#107466
is fixed in std but not in crossbeam-channel.

I originally reworked this because the bug in 80ff3ff
made me desparate to suspect something is wrong with the
channel. While it didn't turn out to be an issue with the
channels  I still think it's good to depend on std code
instead of a library.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz added a commit to feldera/feldera that referenced this issue Jun 23, 2024
The code is identical except that
rust-lang/rust#107466
is fixed in std but not in crossbeam-channel.

I originally reworked this because the bug in 80ff3ff
made me desparate to suspect something is wrong with the
channel. While it didn't turn out to be an issue with the
channels  I still think it's good to depend on std code
instead of a library.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz added a commit to feldera/feldera that referenced this issue Jun 23, 2024
The code is identical except that
rust-lang/rust#107466
is fixed in std but not in crossbeam-channel.

I originally reworked this because the bug in 80ff3ff
made me desparate to suspect something is wrong with the
channel. While it didn't turn out to be an issue with the
channels  I still think it's good to depend on std code
instead of a library.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz added a commit to feldera/feldera that referenced this issue Jun 23, 2024
The code is identical except that
rust-lang/rust#107466
is fixed in std but not in crossbeam-channel.

I originally reworked this because the bug in 80ff3ff
made me desparate to suspect something is wrong with the
channel. While it didn't turn out to be an issue with the
channels  I still think it's good to depend on std code
instead of a library.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz added a commit to feldera/feldera that referenced this issue Jun 24, 2024
The code is identical except that
rust-lang/rust#107466
is fixed in std but not in crossbeam-channel.

I originally reworked this because the bug in 80ff3ff
made me desparate to suspect something is wrong with the
channel. While it didn't turn out to be an issue with the
channels  I still think it's good to depend on std code
instead of a library.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants