Thread-safety mutable cross-references in Rust

The code you posted deadlocks because you lock the game mutex twice:

• Once in main when you call game.lock(),
• And then in register_player when you call game_arc.lock().

Moreover from an API standpoint, needing to pass the game twice to register_player (once as self and once as game_arc) is pretty awkward.

Now if we get rid of the mutex around Game so that we have an Arc<Game>, then we can declare register_player as:

pub fn register_player (self: &Arc<Game>, player_arc: Arc<Mutex<Player>>)

Playground

But then we need to put a Mutex inside Game if we want to be able to mutate it:

pub struct Game {
    players: Mutex<Vec<Arc<Mutex<Player>>>>,
    // Here: ^^^^^
}

In this case Game contains a single field so it's easy, but assuming your game state is more complex, this becomes painful to use.

It would be nice if we could tell the compiler that register_player takes an &Arc<Mutex<Self>>, but unfortunately we can only use types that Deref to Self and Mutex can't (*). We also can't add the register_player method directly on Mutex<Game> like this:

// Not allowed:
impl Mutex<Game> {
    pub fn register_player (self: &Arc<Mutex<Game>>, player_arc: Arc<Mutex<Player>>) {
        todo!();
    }
}

because Mutex is a foreign type. We can however fake it using an extension trait:

pub trait GameExt {
    pub fn register_player (self: &Arc<Self>, player_arc: Arc<Mutex<Player>>);
}

impl GameExt for Mutex<Game> {
    fn register_player (self: &Arc<Self>, player_arc: Arc<Mutex<Player>>) {
        todo!();
    }
}

Finally, having an Arc<Player> inside Game and an Arc<Game> inside Player is a bad idea because it risks introducing memory leaks: they will still refer to each other even when you drop all other references, so their respective reference counts will never reach 0 and they will never be freed. This can be avoided by replacing one of the Arcs by a Weak pointer, which will break the cycle.

Full working example (I also removed a bunch of superfluous muts since Arc<Mutex<_>> values don't need to be mut before they're locked):

use std::sync::{Arc, Mutex, Weak};

pub struct Player {
    pub x: f32,
    pub y: f32,
    game: Option<Weak<Mutex<Game>>>,
}

impl Player {
    pub fn create (x: f32, y: f32) -> Arc<Mutex<Self>> {
        let player = Player {
            x,
            y,
            game: None,
        };
        Arc::new (Mutex::new (player))
    }

    pub fn mount_game (&mut self, game: Arc<Mutex<Game>>) {
        self.game = Some (Arc::downgrade (&game));
    }
    
    pub fn modify_game_state (&self) {
        self.game.as_ref().unwrap().upgrade().unwrap().lock().unwrap().state = 42;
    }
}

pub struct Game {
    state: i32,
    players: Vec<Arc<Mutex<Player>>>,
}

impl Game {
    pub fn new() -> Arc<Mutex<Self>> {
        let game = Game {
            state: 0,
            players: Vec::new(),
        };
        Arc::new (Mutex::new (game))
    }
}

pub trait GameExt {
    fn register_player (self: &Arc<Self>, player_arc: Arc<Mutex<Player>>);
}

impl GameExt for Mutex<Game> {
    fn register_player (self: &Arc<Self>, player_arc: Arc<Mutex<Player>>) {
        player_arc.lock().unwrap().mount_game (Arc::clone(self));
        let mut game = self.lock().unwrap();
        game.players.push (player_arc);
    }
}

fn main() {
    let p1 = Player::create (0.0, 0.0);
    let p2 = Player::create (0.0, 0.0);
    let game = Game::new();
    game.register_player (p1); // or Arc::clone (&p1) if you need to use p1 later in this function
    game.register_player (Arc::clone (&p2));
    p2.lock().unwrap().modify_game_state();
}

Playground

(*) Mutex<Foo> can't Deref to Foo because it needs somewhere to put the MutexGuard while the reference is live, and Deref doesn't offer anywhere to put the guard.