Feature: better support for single-thread runtimes

[hellertime]

The current implementation relies on a sync::Mutex which is more than is needed when using async-shutdown on a single threaded runtime.

Letting the user choose between single-thread and multi-thread modes would be helpful.

I have made a PR #10 that is a working implementation of this idea, and have been using it on my own projects.

I hope there is interest in taking the patch upstream, as it touches every location where the lock is currently taken, and means that rebasing to newer upstream versions is a significant amount of work, but with this patch in place, future version increases will be much simpler to accept.

[de-vri-es]

hey! Thanks for your contribution.

Could you elaborate a bit on the trade-off for your use case?

It's true that the mutex is more than needed, but does it cause a performance bottleneck? Obtaining an unlocked mutex should be very fast thanks to user-space mutexes.

[hellertime]

The performance differences are negligible (at least on my particular hardware), what I wanted to enforce was the !Sync nature of the single-thread model.

[de-vri-es]

Why do you want it to be !Sync though? You can still use a Sync object in a single threaded runtime. The lock is always only held briefly, and never across an await point. So it should never cause a deadlock.

Did you observe a deadlock with the Mutex implementation?

[hellertime]

I'm only wanting the !Sync design to be cautious.

In my case I have a service that spawns multiple threads, one per core. If someone happens to go and refactor the code and moves the ShutdownManager up to the wrong layer, and clones it among threads then it will suddenly cause unexpected interactions.

Having this added language support to catch this case was worth it for me.

[de-vri-es]

Hmm. But you would need to be !Send too right? Otherwise the shutdown manager can still be cloned and sent to other threads.

Either way, I'm hesitant to add a lot of complexity just for removing Sync (or Send). Can't you simply wrap the ShutdownManager in something that is not Sync if you really want to prevent misuse?

[hellertime]

RefCell is !Send, that should make the ShutdownManager also !Send, assuming the inner is Rc<RefCell<T>> and not Arc<Mutex<T>>.

Also, I would claim that the LockGuard concentrates the complexity, and reduces the init to a simple LockGuard::new instead of an Arc::new(Mutex::new()), and the call sites to a borrow() instead of a lock().unwrap(),

but this is just personal preference, since you are the maintainer, I can't say my choice is more valid than yours here.

personally I'd rather not add a wrapper, and have no need for the Arc<Mutex<T>> so I'll likely maintain my fork for now, and will be OK with whatever decision you make here.

[de-vri-es]

Yeah, it's not the complexity of having a Rc<RefCell> instead of a Mutex, or the adjusted implementation that I'm worried about.

What I'm concerned about is having the inner pointer type configurable through a feature flag.

For one, feature flags are unified across the dependency tree, so in general features should be additive. You could argue this one is, since it makes the whole thing Send and Sync. But one of your dependencies could in principle enable the feature and you loose your !Send, !Sync guarantee.

Another option is using a generic argument and a trait for the inner pointer type. This also complicates things for the user, but maybe a default for the generic argument could hide that complexity, like the Allocator of Vec.

Something like:

struct ShutdownManager<T, Lock = lock::ArcMutex>
where
  Lock: lock::Lock
{
  inner: Lock,
}

with lock.rs:

pub trait Lock {
  type LockGuard: Deref<Target = crate::ShutdownManagerInner> + DerefMut;

  fn lock(pointer: &self) -> Self::LockGuard;
}

pub struct ArcMutex {
  inner: Arc<Mutex<crate::ShutdownManagerInner>>,
}

pub struct RcRefCell {
  inner: Rc<RefCell<crate::ShutdownManagerInner>>,
}

impl Lock for ArcMutex {
  type LockGuard = MutexGuard<crate::ShutdownManagerInner>;

  fn lock(&self) -> Self::LockGuard {
    pointer.lock().unwrap()
  }
}

impl Lock for RcRefCell {
  type LockGuard = RefMut<crate::ShutdownManagerInner>;

  fn lock(&self) -> Self::LockGuard {
    pointer.borrow_mut()
  }
}

[hellertime]

Yes, the feature flag approach is admittedly clunky, and in my fork I actually had "multi-thread" be opt-in, I swapped it in the PR since I was trying to retain default behavior, but perhaps I should have gated the single thread mode under the flag instead (maybe as a "not_send" feature).

The defaulted generic approach also seems like a fine solution, that has a similar feel to how the metered crate provides thread safe and not thread safe versions of its metric types: https://docs.rs/metered/0.9.0/metered/common/struct.HitCount.html, and doing away with the feature flag is a good thing.

Can Lock really avoid the generic T though?

[de-vri-es]

I think we can avoid the generic T because we only need it to wrap ShutdownManagerInner. If we have to expose the wrapped object then we would also need to make ShutdownManagerInner public, which I really would prefer to avoid.

The alternative is a generic const, but that one feels more clunky:

trait Lock {
  type Pointer<T>;
  type LockGuard<T>: Deref<Target = T> + DerefMut;

  fn lock<T>(pointer: &Self::Pointer<T>) -> Self::LockGuard<T>;
}

#[non_exhaustive]
struct ArcMutex;

impl Lock for ArcMutex {
  type Pointer<T> = Arc<Mutex<T>>;
  type LockGuard<T> = MutexGuard<T>;

  fn lock<T>(pointer: &Self::Pointer<T>) -> Self::LockGuard<T> {
    pointer.lock().unwrap()
  }
}

The thing I dont like with this one is we need a struct just represent the concept of a Arc<Mutex<T>> instead of a real object with data.

[hellertime]

Yeah exposing the Inner would not be something desirable.

Currently ShutdownManagerInner<T>, since it owns the Option<T> holding the shutdown_reason.

I echo your desire to not have marker structs if it can be avoided, but I'm still not clear on how the original suggestion sidesteps needed to expose the T?

[de-vri-es]

This is how:

pub struct ArcMutex {
  inner: Arc<Mutex<crate::ShutdownManagerInner>>,
}

We simply wrap Arc<Mutex<crate::ShutdownManagerInner>> in a struct called ArcMutex to "hardcode" T = ShutdownManagerInner.

[hellertime]

The ShutdownManagerInner is currently defined in terms of T, that will require the Lock to remain in terms of T unless there is some other way to fix T, thats what I'm not understanding at the moment.

Sorry, I'm not trying to be intentionally dense, I'm still only a year or so into writing rust, its very possible I'm missing something obvious here.

[de-vri-es]

Oh, sorry. I totally forgot about that generic. Either way, no need to apologize, even if it wasn't my mistake :)

Hmm, then the only thing that would avoid repeating the T is the second suggestion the the generic associated type :[

[hellertime]

Yeah, but that isn't all that terrible, its not really a new burden to expose the T. And its still reasonably concise, if some new third type of lock was desired.

And it does away with the feature flag without making it a runtime decision, which is the real plus here.

[de-vri-es]

Right, exposing T is fine. But doing it twice is a bit of a shame. Especially when it's a long type name.

Considering that, I am in favor of the solution with a generic associated type and an empty struct to represent the concept of the lock rather than the actual lock.

So something along the lines of this:

trait Lock {
  type Pointer<T>;
  type LockGuard<T>: Deref<Target = T> + DerefMut;

  fn lock<T>(pointer: &Self::Pointer<T>) -> Self::LockGuard<T>;
}

#[non_exhaustive]
struct ArcMutex;

impl Lock for ArcMutex {
  type Pointer<T> = Arc<Mutex<T>>;
  type LockGuard<T> = MutexGuard<T>;

  fn lock<T>(pointer: &Self::Pointer<T>) -> Self::LockGuard<T> {
    pointer.lock().unwrap()
  }
}

Added benefit is that people can implement the Lock trait themselves, since it doesn't require knowing about ShutdownManagerInner anymore.