&self vs. &mut self for spawners

[stbuehler]

Hi,

I'd like to reopen this discussion (#767 before for executors).

@aturon wrote in #767 (comment)

I think I'm convinced we should go with &mut self. Originally I had been thinking that with &self we could allow users of executors to decide on the memory management strategy (by applying an Rc/Arc themselves), but in practice I think these are usually baked into the executor anyway.

Exactly! The strategy is already baked in (Spawn/LocalSpawn implementations usually implement Clone too!) - so there is really no need for &mut self.

It would be way safer though to store a &self reference in TLS for the "current spawner".

[Nemo157]

Do you mean to change both Spawn and LocalSpawn to take &self, or just Spawn?

It seems like there could definitely be a performance benefit from having a LocalSpawn + !Clone + !Send implementation that can just directly access the current executors state to spawn a new task. It's much more limiting since you can only spawn new tasks from a single location, but sometimes that's all that's needed.

---

When I was playing around with global spawners I ended up using an internal trait for this

trait SharedSpawn {
    fn spawn_obj(&self, future: FutureObj<'static, ()>) -> Result<(), SpawnError>;

    fn status(&self) -> Result<(), SpawnError>;
}

impl<Sp> SharedSpawn for Sp where for<'a> &'a Sp: Spawn {
    fn spawn_obj(mut self: &Self, future: FutureObj<'static, ()>) -> Result<(), SpawnError> {
        Spawn::spawn_obj(&mut self, future)
    }

    fn status(&self) -> Result<(), SpawnError> {
        Spawn::status(&self)
    }
}

that allowed the global setter function to take in a single instance of the spawner and access that via an RwLock (since it's almost certainly going to just be set once at application startup).

fn set_global_spawner<Sp: Send + Sync + 'static>(spawner: Sp) where for<'a> &'a Sp: Spawn

The nice thing of this sort of pattern is it allows for both unique and shared spawners to exist behind the same basic trait, it's up to the implementor to implement it for shared references if they don't require unique access.

[stbuehler]

I mean both Spawn and LocalSpawn.

I don't see a real performance benefit for !Clone + !Send - I'm pretty sure it will be almost always safe to simply wrap it in UnsafeCell: once you're in spawn_local_obj the local thread shouldn't be able to re-enter it (given the implementation should be rather short and not calling anything unexpected). Or wrap it in RefCell for almost no cost to be safe.

And given how hard it will be to pass such spawner around I'm not sure you'll actually need a trait for it.

[stbuehler]

On a related note I'd still be curious if there is a safe way to pass a mutable reference through pointers in TLS...

let p: *mut T = transmute(mut_ref);
// through some callback
{ let r: &mut T = transmute(p); use(r); }
otheruse(mut_ref);

I don't see what would prevent the compiler from inlining use and otheruse and mixing the access to the referenced object - lifetimes will look "safe" from the compilers perspective. Probably needs compiler_fence(...)?

[seanmonstar]

FWIW, the same question is being considered in tokio right now: tokio-rs/tokio#1344

[qnighy]

I want the SharedSpawn workaround to be integrated into futures-rs. Another approach would be to make SharedSpawn a subtrait of Spawn and do similar things as Fn/FnMut distinction.

[stbuehler]

I don't think the library is in a life-cycle stage that warrants "workarounds" like this: just "fix" (i.e. change) it properly.

I don't see any open argument why the spawn traits need &mut self (unless @Nemo157 is not happy with my answer above?).

[Nemo157]

#1763 doesn't look like a "workaround" to me, it seems like a pretty straightforward way for users and implementers to precisely define whether they need unique access.

I just recalled one implementation that does need unique access: impl Spawn for FuturesUnordered. I don't know if this is used anywhere currently though (and requiring unique access seems like it makes using it potentially difficult in some circumstances).

[LucioFranco]

@Nemo157 one thing with futuresunordered is I think it may become more useful with the combination of select! and async/await. I know it could be super useful for libraries that need to spawn like tower-hyper and take an executor/spawner.

[stbuehler]

I think FuturesUnordered is one of those that could simply use a RefCell (or UnsafeCell) internally (and drop Sync). Right now it is Sync (if the underlying futures are Sync), but I don't see what that is useful for.

And I really don't think it's gonna be "straightforward [..] for users and implementers to precisely define whether they need unique access". As a "user" of the trait (i.e. an interface that wants a "Spawner" as input), do I use "UniqueSpawn" or "SharedSpawn"? Maybe right now I don't need to share it, so "UniqueSpawn" seems acceptable, but it'll limit me "forever" (until I break the API). That is why I think adding more traits without good reason is a bad design choice.

[cramertj]

FuturesUnordered does rely on unique access for the len and head_all members of the type, which are modified when new elements are added to the linked list. These could be changed to be atomic, but not freely so.

[cramertj]

I did find the trait argument suggestion in tokio-rs/tokio#1344 pretty compelling, though that could perhaps be accomplished via a second trait and blanked impl, but that's pretty uncomfortable.

[stbuehler]

@cramertj I think I already pointed out the solution to FuturesUnorderd directly above your post - unless you disagree with it, but then I'd like to hear why :) In short: I don't think FuturesUnordered needs to be Sync, and Send shouldn't be a problem.

[stbuehler]

Also I'd be willing to (try to) PR this it if this sounds acceptable.

[cramertj]

Ah, I see what you're saying now. Yes, that makes sense to me, and I'll put together a PR. My one concern would be whether users would want to have a reference to a FuturesUnordered across an .await boundary, but I'd expect holding a mutable reference to be common, and holding a shared reference to a FuturesUnordered would be very uncommon.