Thread-safety mutable cross-references in Rust

Question

I want to implement two different structures that have references to each other, can mutate each other, and are thread-safety.

I can introduce the following example:

pub struct Player {
    pub x: f32,
    pub y: f32,
    game: Option<Game>, // Reference to `Game`
}

impl Player {
    pub fn new(x: f32, y: f32) -> Self {    
        Player { x, y, game: None}
    }

    // Some functions that can change `self`, `game`, and `map`
}

pub struct Game {
    pub map: GameMap,
    players: Vec<Player>, // Reference to `Player`
}

impl Game {
    pub fn new() -> Self {
        Game { map: GameMap::new(), players: Vec::new()}
    }

    pub fn register_player(&mut self, player: Player) {
        todo!();
    }
}

I'd like to have a similar interface:

fn main() {
    let mut p1 = Player::new(0.0, 0.0);
    let mut p2 = Player::new(100.0, 100.0);

    let mut game = Game::new();
    game.register_player(p1);
    game.register_player(p2);

    p1.forward();  // changes its coordinates using `map` from `game`
    p2.shoot();  // changes the `map` and possibly another player
}

I can't use std::rc::Rc and std::cell::RefCell because my goal is to write thread-safety code. I tried std::sync::{Arc, Mutex}, but it's not succeeded yet. How could I solve this challenge?

---

UPD Here I add the code where I tried to use mutex

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

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

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

    pub fn mount_game(&mut self, game: Arc<Mutex<Game>>) {
        self.game = Some(game);
    }
}

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

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

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

fn main() {
    let mut p1 = Player::create(0.0, 0.0);
    let mut p2 = Player::create(0.0, 0.0);
    let mut game = Game::create();
    game.lock().unwrap().register_player(Arc::clone(&game), Arc::clone(&p1));
    game.lock().unwrap().register_player(Arc::clone(&game), Arc::clone(&p2));
}

Answer 1

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.