DashMap footguns

[plebhash]

during the review process of #299 + #304 we're coming to the realization that DashMaps have footguns

more specifically, holding locks/guards across await points, which compiler/clippy/fmt never warns us about

we need to come up with a sustainable strategy for this

the two main approaches that come to mind:

• not use DashMap
• establish some kind of convention/policy on CONTRIBUTING.md that forces us to have extra discipline and sanity-check PRs as much as possible against these footguns

[bit-aloo]

In our codebase we often define struct and pass around an Arc<Mutex> in the code, with lock calls all over the place. This is a bad idea for several reasons:

1. When reading the code, lock calls are unnecessary clutter.
2. Every time a method takes the collection by argument, we have to write an unnecessary Arc/Mutex in the type signature. This is also clutter.
3. You are leaking implementation details. For example, if you want to swap the Mutex for an RwLock, you have to change every single use of the shared collection across the entire codebase.

We can easily hold the mutex locked for too long by accident. This is a particularly bad issue in async code.

[bit-aloo]

I am thinking of having something like this:

use std::collections::DashMap;
use std::sync::Arc;

#[derive(Clone)]
pub struct SharedMap {
    inner: Arc<SharedMapInner>,
}

struct SharedMapInner {
    data: DashMap<i32, String>,
}

impl SharedMap {
    pub fn new() -> Self {
        Self {
            inner: Arc::new(SharedMapInner {
                data: DashMap::new(),
            })
        }
    }

    pub fn insert(&self, key: i32, value: String) {
        self.data.insert(key, value);
    }

    pub fn with_value<F, T>(&self, key: i32, func: F) -> T
    where
        F: FnOnce(Option<&str>) -> T,
    {
        func(self.data.get(&key).map(|string| string.as_str()))
    }
}

Something very similar on same lines even for types which we wrap inside Arc<Mutex<...>>, having a wrapper type which can provide set and get methods without being explicit about the lock pattern underneath.

[bit-aloo]

Let me try to sketch this on my Pool DashMap PR, lets see how the pattern gonna look.

[bit-aloo]

I implemented the above suggestion on the dashmap PR: a1f7dbf , and it kinda look good with strong assertions and no footguns.

[bit-aloo]

In this change, I introduced Shared and SharedMap. The former is an abstraction over Arc<Mutex<...>>, and the latter wraps Arc<DashMap<...>>. Both provide simple accessor methods around these underlying constructs.

The motivation behind this is threefold:

1. It avoids the recurring guard-related issues.
2. It enables efficient concurrent access through DashMap.
3. It removes the need for super_safe/safe_lock conventions.

Overall, the API remains straightforward, and I believe it helps make the codebase leaner while reducing potential footguns.

As for unifying these abstractions under a single interface, I am not entirely convinced it would be beneficial. To me, it introduces additional complexity by trying to expose a single surface over fundamentally different data structures. I would also prefer to avoid introducing trait-heavy generic abstractions here, as they can be difficult to design cleanly and ensure correctness across all use cases.

[plebhash]

cool, after looking into the app-layer APIs during vinteum OH, as we discussed, I think this is going in an interesting direction

As for unifying these abstractions under a single interface, I am not entirely convinced it would be beneficial. To me, it introduces additional complexity by trying to expose a single surface over fundamentally different data structures. I would also prefer to avoid introducing trait-heavy generic abstractions here, as they can be difficult to design cleanly and ensure correctness across all use cases.

I guess I wasn't fully clear there. I'm not arguing for squashing stratum_apps::shared::Shared + stratum_apps::shared::SharedMap

What I meant was having stratum_apps::custom_mutex::Mutex as a standalone modularized abstraction that is embedded into stratum_apps::shared::Shared feels like a bad idea.

If we end up going in this direction, IMO we should completely eliminate stratum_apps::custom_mutex::Mutex and have whatever logic we want to stay implemented directly inside stratum_apps::shared::Shared.

---

but one question remains: what about RwLock?

[plebhash]

but one question remains: what about RwLock?

as discussed on OH, that means we would have two variants:

• stratum_apps::shared::Shared
• stratum_apps::shared::SharedRw

[bit-aloo]

Based on the discussion, e0a0f75, this commit should cover all the possible scope of our current usage of shared synchronous data structure.

[bit-aloo]

This will also lead to deprecation of custom_mutex