Implement Ord for task::Id

[tyilo]

Motivation

I want to use the id from a JoinSet as a key in a BTreeMap.

Solution

Implement PartialOrd and Ord for Id.

[ADD-SP]

Could you elaborate your original requirements? Why do you want to save task id in an ordered map? Without this ordered map, what difficulties are hard to solve?

Currently, we only guarantee the uniqueness of task ids, their order has no special meaning.

[tyilo]

I don't really care about the ordered property of a BTreeMap, but I think it would be faster than a HashMap.

For instance both std::io::ErrorKind and std::any::TypeId implements Ord even though they have no "natural" ordering.

See also C-COMMON-TRAITS

---

The concrete code I wanted it for is this (with HashMap replaced by BTreeMap):

use std::collections::HashMap;
use std::hash::Hash;

use tokio::task::JoinSet;

trait InsertOnce<K>
where
    K: ?Sized,
{
    type V: ?Sized;

    fn insert_once(&mut self, key: K, value: Self::V);
}

impl<K, V> InsertOnce<K> for HashMap<K, V>
where
    K: Eq + Hash,
{
    type V = V;

    fn insert_once(&mut self, key: K, value: Self::V) {
        let previous = self.insert(key, value);
        assert!(previous.is_none());
    }
}

impl<V> InsertOnce<usize> for Vec<Option<V>> {
    type V = V;

    fn insert_once(&mut self, key: usize, value: Self::V) {
        let entry = &mut self[key];
        assert!(entry.is_none());
        *entry = Some(value);
    }
}

pub async fn spawn_and_join_all<I, F, T>(futures: I, max_concurrent: usize) -> Vec<T>
where
    I: IntoIterator<Item = F>,
    F: Future<Output = T> + Send + 'static,
    T: Send + 'static,
{
    let mut futures = futures.into_iter().fuse();

    let mut set = JoinSet::new();
    let mut ids_to_index = HashMap::new();

    for f in futures.by_ref().take(max_concurrent) {
        let id = set.spawn(f).id();
        ids_to_index.insert_once(id, ids_to_index.len());
    }

    let mut total_started = set.len();
    let lower_bound = total_started + futures.size_hint().0;
    let mut result: Vec<Option<T>> = Vec::with_capacity(lower_bound);
    result.resize_with(total_started, Default::default);

    while let Some(r) = set.join_next_with_id().await {
        let (id, v) = r.unwrap();
        let index = ids_to_index.remove(&id).unwrap();
        result.insert_once(index, v);

        if let Some(f) = futures.next() {
            let id = set.spawn(f).id();
            ids_to_index.insert_once(id, total_started);
            total_started += 1;
            result.push(None);
        }
    }

    result.into_iter().map(|v| v.unwrap()).collect()
}

[ADD-SP]

I believe that now we have two topics.

Task ID may be reused

    fn insert_once(&mut self, key: K, value: Self::V) {
        let previous = self.insert(key, value);
        assert!(previous.is_none());
    }

// ----- omitted -----

        if let Some(f) = futures.next() {
            let id = set.spawn(f).id();
            ids_to_index.insert_once(id, total_started);
            total_started += 1;
            result.push(None);
        }

There is a potential panic case.

Task IDs are unique relative to other currently running tasks. When a task completes, the same ID may be used for another task.

tokio::task::Id

Add PartialOrd and Ord for convenience

Thanks for your mentioning std::io::ErrorKind and std::any::TypeId, I searched their PRs and read the discussion.

• Implement more traits for std::io::ErrorKind (Implement more traits for std::io::ErrorKind rust-lang/rust#35911)
• Add PartialOrd, Ord derivations to TypeId (Add PartialOrd, Ord derivations to TypeId rust-lang/rust#38981)

I now think it makes sense to add these two features to make things easier (even they don't have natural ordering) for downstream users so I am not opposed to this change at this point.

However, firstly, it would be better taking care of the potential panic I mentioned above, then we can re-evaluate do we actually need a PartialOrd in your case.

Again, I am not opposed to this change after reading the above two PRs, but I still want to keep it as it is until there is a use case that absolutely requires this change.

[tyilo]

There is a potential panic case.

Not really, because of the call to ids_to_index.remove(&id).

[ADD-SP]

Looks good to me, I also learned some interesting stories today.

Since merging this PR will make PartialOrd and Ord be the part of semantic version guarantee, I would like to keep this PR open for a few days so that the community and other maintainers can participate in our discussion during this time window.

[carllerche]

Given the TypeId precedence, I am fine adding Ord / PartialOrd implementation. I think it is fairly safe.

[Darksonn]

Thanks.