Warn against usage of this library

[SabrinaJewson]

A common mistake for async beginners to make is using this library. I propose that this library is a proof-of-concept and should not be used in production code. In particular:

• The scope_and_block API prevents composability, meaning your function cannot be called in an async function. But there is no compiler error to prevent this, so users can easily make the mistake of blocking the thread, which leads to difficult-to-debug deadlocks.
• Even if one uses this API correctly, it is borderline useless, because it can only be used at the top level, and at that level it’s typically very easily to just make your data 'static anyway. Using it also makes it much harder to refactor an async function.
• The scope API is unsafe, which is good, but I have multiple times seen people claim that they’re using it in a safe manner, when they are in fact not. The truth is that because you can’t control what the caller does with the future, it is in fact impossible to correct use in an async function without making the async fn itself unsafe, which just forwards the easily-misunderstood safety preconditions to the caller. Of course, you could always heap allocate 'static data, but at that point you may as well just Arc it and this crate becomes unnecessary.

Given these points, I believe this crate should clearly advertise that it is not recommended for use for purposes other than education in the README and documentation.

[vbauerster]

Hi @SabrinaJewson!
I'd like to rewrite following thread::scope snippet to tokio::spawn equivalent for educational purposes. How can I accomplish this without using this library? Can you please give me a hint?

use std::thread;

fn main() {
    let max = num_cpus::get();
    let mut data = vec![0; 32];

    thread::scope(|s| {
        for (i, chunk) in data.chunks_mut(max).enumerate() {
            s.spawn(move || {
                for (j, elem) in chunk.iter_mut().enumerate() {
                    *elem = collatz_steps(max * i + j + 1);
                }
            });
        }
    });

    println!("{:?}", data);
}

fn collatz_steps(mut n: usize) -> usize {
    let mut steps: usize = 0;
    while n > 1 {
        n = collatz_next(n).expect("overflow");
        steps += 1;
    }
    steps
}

fn collatz_next(n: usize) -> Option<usize> {
    if n % 2 == 0 {
        Some(n / 2)
    } else {
        n.checked_mul(3).and_then(|n| n.checked_add(1))
    }
}

[SabrinaJewson]

I mean, just on first glance there are a few things wrong with that code. First of all, it seems the intent was to spawn as many threads as there are CPU cores, but what the code actually does is spawn potentially many more threads, with each one processing as many items of data as there are CPU cores. Maybe what you wanted is .chunks_mut(data.len() / num_cpus::get()) instead?

(As a side note, num_cpus is effectively obsolete since available_parallelism now exists in std.)

Second of all, if your code is CPU-bound like this, it is generally better to use Rayon instead of Tokio. Though if you really want to use Tokio, you can, and a simple way is just to have each task own its own Vec:

use std::thread::available_parallelism;

#[tokio::main]
async fn main() {
    let threads = available_parallelism().unwrap().get();

    let mut tasks = Vec::new();

    let total_len = 32;
    let batch_size = total_len / threads;
    for i in 0..threads {
        let start = i * batch_size;
        let len = Ord::min(start + batch_size, total_len) - start;
        tasks.push(tokio::spawn(async move {
            let mut data = vec![0; len];
            for (j, elem) in data.iter_mut().enumerate() {
                *elem = collatz_steps(start + j + 1);
            }
            data
        }));
    }

    let mut data = Vec::new();
    for task in tasks {
        data.extend(task.await.unwrap());
    }

    println!("{:?}", data);
}

fn collatz_steps(mut n: usize) -> usize {
    let mut steps: usize = 0;
    while n > 1 {
        n = collatz_next(n).expect("overflow");
        steps += 1;
    }
    steps
}

fn collatz_next(n: usize) -> Option<usize> {
    if n % 2 == 0 {
        Some(n / 2)
    } else {
        n.checked_mul(3).and_then(|n| n.checked_add(1))
    }
}

[vbauerster]

@SabrinaJewson thanks for your help and code snippet! It is very helpful for me.
My intent was to translate golang.org/x/sync/semaphore example to tokio equivalent and the code I provided was my first buggy attempt :) Thanks for mentioning Rayon that would be excellent match for this kind of problem and good exercise for me.

[cdbennett]

It would be interesting to compare/contrast this library to a couple of other crates:

• moro
• async-nursery

These crates implement Structured Concurrency, but use a design is inspired by the Python Trio project, which is rather mature and widely-used.

[rmanoka]

@SabrinaJewson Your comments are well received; indeed these are shortcomings of the crate. However, the unsafety of the APIs and the thread blocking nature of the safe API is clearly advertised, and I do not believe it warrants any proclamation on the README on usage beyond this. I'll leave this issue open to promote discussions on this.

@cdbennett moro as I understand runs the tasks concurrently but not in parallel, here, the tasks run parallelly. Pls. refer this nice explanation from the author. This is also why we had to go for blocking the thread as the only safe way to prevent the stack from being forgotten.

[SabrinaJewson]

However, the unsafety of the APIs and the thread blocking nature of the safe API is clearly advertised, and I do not believe it warrants any proclamation on the README on usage beyond this.

I mean, this request was born from multiple times seeing people attempt to use this library in their code, and me having to tell them that they shouldn’t. It ultimately just feels like a waste of both our time – yes, maybe the intelligent reader would be able to tell that they shouldn’t use this from the documentation, but there are a lot of async beginners out there whom I wouldn’t expect to know that, and they might easily end up feeling confused or stupid because they’re having trouble with their code – this doesn’t seem like a very friendly newcomer experience for async Rust.

Concretely, I might suggest the following paragraph be added to the readme / crate docs:

Limitations

This crate mostly exists as a proof-of-concept, and comes with several inherent limitations. First of all, the only non-unsafe API is blocking, which is easy to misuse (e.g. calling it in an async context will result in deadlocks) and limits composability (since it can only be used in main or in a wholly new thread). And second, the unsafe APIs are more unsafe than they seem – using them correctly involves having control over every part of the call stack above them. This means any async function that calls scope must itself be unsafe, and your reasoning about safety has to be global across the program. This sort of reasoning is, however, rather antithetical to Rust’s principles.

If you think you need scope, it is usually recommended to instead do one of the following:

1. Make your data 'static by wrapping everything in an Arc. This might seem like it has a performance impact, but this is a constant-time cost and thus mostly negigible in practice. To control multiple tasks in a structured way, it is recommend to use a type like Tokio’s JoinSet.
2. Use FuturesUnordered, which provides concurrency but not parallelism. This is often okay if parallelism is provided at a more coarse-grained level anyway.

[sunshowers]

I use async-scoped pretty heavily in nextest, and I've found it to be a great fit. The blocking API is completely okay for me because the test run itself acts as a natural bound. For nested scopes I have to use unsafe but it's not too bad overall, and it makes some kinds of test runs notably faster than FuturesUnordered. (I haven't tried making the data 'static because before I converted it to async, it heavily used borrows.)

So thank you @rmanoka for maintaining async-scoped!

[rmanoka]

@sunshowers feel free to open a feature request issue on the recursive-usage of async-scoped mentioned in the comment.