MPSC channel panic

code review
1

Today I was testing the new version of rust 1.67.0 the Rust Blog was announcing updates on std::sync::mpsc, so I decided to test (after rustup update). Not sure what is wrong but it panics also at the same time gives a result.

use std::sync::mpsc::{channel, Receiver, Sender};
use std::thread;

// Sends the Fibonacci sequence into the channel until it becomes disconnected.
fn fibonacci() -> (Sender<u64>, Receiver<u64>) {
    let (tx, rx) = channel::<u64>();

    let tx_thread = tx.clone();

    thread::spawn(move || {
        let (mut x, mut y) = (0, 1);
        while tx_thread.send(x).is_ok() {
            let tmp = x;
            x = y;
            y += tmp;
        }
    });

    (tx, rx)
}

fn main() {
    let (_, r) = fibonacci();

    // Print the first 20 Fibonacci numbers.
    for num in r.iter().take(20) {
        println!("{num}");
    }
}

This is the result after run cargo run

➜  channel_test git:(master) ✗ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/channel_test`
0
thread '1
<unnamed>1
' panicked at '2
attempt to add with overflow3
', 5
src/main.rs8
:13
1521
:34
13
55
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
89
144
233
377
610
987
1597
2584
4181
2

You're using an unbounded channel. The tx_thread is running much faster than the printing thread, and keeps going until the additions get bigger than u64::MAX. This also allocates lots of memory for the unused values.

To fix it, call mpsc::sync_channel() with a reasonable buffer size, instead of mpsc::channel(), so that the tx_thread will block when the channel is full.

It would also be good to add protection by using checked_add() instead of += and stopping the loop when it fails. Otherwise if it continued, and the program was built with the release profile, the value would wrap around and become nonsense (not the Fibonacci sequence).

5 Likes
3

Thank you for this review. Also, I would like to ask if this kind of construction is some "anti-pattern" in Rust, it was a little inspired by Go generator constructor.

Here is the new version of the snippet

use std::sync::mpsc::{sync_channel, Receiver};
use std::thread;

// Sends the Fibonacci sequence into the channel until it becomes disconnected.
fn fibonacci() -> Receiver<u64> {
    let (tx, rx) = sync_channel::<u64>(0);

    thread::spawn(move || {
        let (mut x, mut y) = (0, 1);
        while tx.send(x).is_ok() {
            let tmp = x;
            x = y;
            y = y
                .checked_add(tmp)
                .expect("values generated should not exceed u64 max size");
        }
    });

    rx
}

fn main() {
    let rx = fibonacci();

    // Print the first 20 Fibonacci numbers.
    for num in rx.iter().take(20) {
        println!("{num}");
    }
}
4

I would definitely say this is very overkill (spinning up a thread) to essentially express an iterator.

The iterator version would be:

struct FibIter {
    x: usize,
    y: usize,
}

impl FibIter {
    pub fn new() -> Self {
        Self {
            x: 0,
            y: 1,
        }
    }
}
impl Iterator for FibIter {
    type Item = usize;
    fn next(&mut self) -> Option<usize> {
        let tmp = self.x;
        self.x = self.y;
        self.y = self.y
                      .checked_add(tmp)?;
        Some(tmp)
    }
}

fn main() {
    for num in FibIter::new().take(20) {
        println!("{}", num);
    }
}

(Note I preserved most of the behaviour, but you could squeeze out a few more fibbonacci numbers in the range of u64 given how this is implemented).

5

Totally agree, that kind of structure just for a generator like that looks like a waste of resource to use a OS thread just to create a generator. So to clarify it was just an experiment using the mpsc structure in the std lib. Few weeks ago I was testing a similar pattern using the tokio runtime with tokio::spawn (which probably not a was since it was a green thread)

6

This will predictably panic. It would be better to simply break the loop (which will exit the thread and close the channel) when the overflow occurs — this could be understood as indicating to the receiver “there are no more fibonacci numbers that fit in a u64”.

1 Like
7

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.