Move closure gotcha with primitives

I was debugging why my application returned the wrong result. I was using async streams to send requests and counting how many bytes was sent, but always got 0.

At first I thought it had something to do with async move as I reduced it down to a simple test case:

async fn do_async() {
    let mut iters = 0;
    async move {
        for _ in 0..3 {
            iters += 1;
            println!("iters={}", iters);
        }
    }.await;
    println!("total iters={}", iters);
}

Playground link

Which prints:

iters=1
iters=2
iters=3
total iters=0

Then I came to the realization that it's not an issue with async move, but move closures in general:

    let mut iters = 0;
    let handle = thread::spawn(move || {
        for _ in 0..3 {
            iters += 1;
            println!("iters={}", iters);
        }
    });
    handle.join().unwrap();
    println!("total iters={}", iters);

Then I found out that there's even an example of it in the rust book:
https://doc.rust-lang.org/1.30.0/book/first-edition/closures.html#move-closures

:facepalm:

So my issue is that this is a very easy mistake to do, inadvertently.

If I understand it correctly, this can happen with any primitives, because primitives implement Copy, so the outer iters variable is a "clone" of the inner one.
In my case async_streams uses async move under the hood, so I had no idea that that was the cause.
My solution was to wrap the variable in an Arc/AtomicUint, which is fine, I just wish it would be easier to discover this.

Is there a way to make this safer/detectable at compile-time?
If it was a Vec or something else non-primitive it would fail with borrow of moved value: iters
Is it possible to make mut variables non-copyable? Or a linter check that could catch this?

It's a gotcha indeed. It is detectable. For example, if you make the compiler analyze the code as if they were separate bindings, then it can see the outer one not mutated:

use std::thread;

fn main() {
    let mut iters = 0;
    let handle = thread::spawn(move || {
        let mut iters = iters; // added
        for _ in 0..3 {
            iters += 1;
            println!("iters={}", iters);
        }
    });
    handle.join().unwrap();
    println!("total iters={}", iters);
}

There are many legitimate reasons to write code like that, so a lint against it could create a lot of false positives. Anybody who creates threads in a loop and passes Copy data into each one will probably do something like this.

I think this is just something that has to be learned. Copy types are copied on move (which is largely the point of Copy), and the code would have been doing multiple concurrent writes if it wasn't a copy, so it's natural to be suspicious of anything passed into multiple mutating threads without being wrapped in Arc.

Additionally, you pointed to async_stream here, which uses a macro to construct closures. (If I understand you correctly.) To be frank, people overuse macros. They tend to make code quicker to write, but often harder to understand.

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.