2

For an implementation of a games browser I need a collection of games that can be read and written by different threads. I want the elements of the collection to be as independent of each other as possible. I feel like changing one game should not disturb another game. Thats why I am hesitating to make use of the ReaderWriterLockSlim class when changing a game. As of now I am using a combination of a simple lock object within the game class and the ReaderWriterLockSlim class as follows:

  • Consumer: Multiple threads. One for every client that requests the collection of games. They are using EnterReadLock.
  • Producer (add): One thread for adding new games to the collection if required. After it finishes its work it will be called through a timer later again. It is using EnterUpgradeableReadLock once and EnterWriteLock for every required new game.
  • Producer (modify): Multiple threads. Up to at least one for every game that exists in the collection. Every thread is always solely going to change one game. There may be multiple threads for one game. To access the right game in the collection they are making use of EnterReadLock. To prevent multiple threads from interfering when processing the same game, I am using an additional simple lock object on the game class right now.

As of this article Are IEnumerable Linq methods thread-safe? I know, that modifying the collection invalidates the enumerator and its behavior becomes undefined. But I am not sure, how to solve it properly.

So my Problem is: Modifying producers may change the elements of the collection while either

  • adding producers may add new elements or
  • consumers may iterate over the collection via linq expressions.

What do you suggest? Does a thread that wants to change a game really have to EnterWriteLock, thus preventing other games from being updated at the same time? Am I too concerned about performance issues?

Sample code:

public class Game
{
    private readonly IList<Player> players;

    public Game(int id, string name, GameMode mode)
    {
        ID = id;
        Name = name;
        Mode = mode;
        Status = GameStatus.WaitingForPlayers;
        players = new List<Player>();
    }

    public object Lock { get; private set; } = new object();
    public int ID { get; }
    public string Name { get; }
    public GameMode Mode { get; }
    public GameStatus Status { get; private set; }
    public int PlayerCount => players.Count;

    public void Join(Player player)
    {
        // Just an example...
        players.Add(player);
    }

    public void Leave(Player player)
    {
        players.Remove(player);
    }

    public void Start()
    {
        // Just an example...
        Status = GameStatus.Started;
    }

    public GameDto ToDto() => new GameDto(ID, Name, Mode, Status, PlayerCount);

    // Just an example. Depends on if it was started already and so on...
    public bool CanStart => PlayerCount > 3;
}

public class Server : IDisposable
{
    private readonly ReaderWriterLockSlim gamesLock = new ReaderWriterLockSlim();

    // This is the collection, that is giving me headaches.
    private ICollection<Game> games = new List<Game>();

    // ... Constructor and more ...

    // Will be called once in a while but not in parallel. Adds games, if available games are full...
    private void AddRequiredGames()
    {
        while (TryGetRequiredGame(GetGames(), out Game game))
        {
            gamesLock.EnterWriteLock();
            games.Add(game);
            gamesLock.ExitWriteLock();
        }
    }

    // Will be called once the last player left a game. There may be multiple threads at once for different games.
    private void RemoveIfObsolete(Game game)
    {
        gamesLock.EnterUpgradeableReadLock();
        lock (game.Lock)
        {
#warning Here I am eventually iterating over the collection, while its elements could be modified.
            if (game.IsEmpty && games.Count(item => item.Mode == game.Mode
              && item.Status == GameStatus.WaitingForPlayers) > 3)
            {
                // ToDo - Mark game as obsolete to prevent incoming join attempts.
                gamesLock.EnterWriteLock();
                games.Remove(game);
                gamesLock.ExitWriteLock();
            }
        }
        gamesLock.ExitUpgradeableReadLock();
    }

    private ICollection<GameDto> GetGames()
    {
        gamesLock.EnterReadLock();
#warning Here I am iterating over the collection, while its elements could be modified.
        IList<GameDto> result = games.Select(item => item.ToDto()).ToList();
        gamesLock.ExitReadLock();
        return result;
    }

    // Will be called asynchronously be every client that requests to join a game.
    private void HandlePlayerJoin(Player player, Game game)
    {
        lock (game.Lock)
        {
#warning Here I am modifying an element of the collection without locking the collection.
            game.Join(player);
        }
        // Just an example. Wouldn't try to start the game, if the player could not join etc...
        TryStartGame(game);
    }

    private void TryStartGame(Game game)
    {
        lock (game.Lock)
        {
            if (game.CanStart)
#warning Here I am modifying an element of the collection without locking the collection.
                game.Start();
        }
    }

    // Will be called asynchronously be every client that requests the list of games.
    private void HandleGetGames(Client client)
    {
        client.Send(new GetGamesResponse(GetGames()));
    }
}
FLav0ured
  • 39
  • 5
  • You can just make your `Game` thread-safe itself, and then all of them into a `ConcurrentDictionary`. This way, you will safely extract \ add \ remove game from collections in a lock-free manner, and then you can safely do whatever you want with your game. You can use `ReaderWriteLockSlim`, if it is mostly read-only operations, but use it inside `Game`, not `Server`. – Yeldar Kurmangaliyev Nov 25 '18 at 16:32
  • In `HandlePlayerJoin` and `TryStartGame` I see no reason you would need to lock around the collection, because you already have a `Game` reference. During this time this `Game` could be removed from the collection, but does that matter to you? – Rotem Nov 25 '18 at 16:32
  • In `RemoveIfObsolete`, you write "Here I am eventually iterating over the collection, while its elements could be modified.", but this is not true, because you are inside a read lock. No other thread can get a write lock while you are inside the read lock. – Rotem Nov 25 '18 at 16:34
  • @Rotem: Thanks for your input. Removing the `Game` reference even temporary from the collection within `HandlePlayerJoin` or `TryStartGame` would mean that players may not see it when invoking `HandleGetGames` during that time. Thats not really what I want. In `RemoveIfObsolete` I am inside a read lock for the collection but as of now `HandlePlayerJoin` could modify the collection anyways, because the method only acquires a lock for its own game whereas `RemoveOfObsolete` could be dealing with a completely different game meanwhile. – FLav0ured Nov 25 '18 at 17:06
  • @Yeldar Kurmangaliyev: Thank your for your advice. Making `Game` thread-safe sounds interesting indeed. I have to think/implement this through, I guess. Same goes for the `ConcurrentDictionary`. I will try it out. – FLav0ured Nov 25 '18 at 17:19
  • `Am I too concerned about performance issues?` - I would say that you probably are. You should verify that you really have an issue before attempting to over-engineer it. `ReaderWriteLockSlim` is extremely fast, albeit not as fast as not locking when there's contention. How frequently (as in number of times per second) will the game list typically change? – 500 - Internal Server Error Nov 26 '18 at 18:00
  • @500-InternalServerError I am not yet finished with the implementation so up until now I can only guess on that. Right now the number of games contained in the list will be changed periodically at least every three seconds depending on how many players join the available games. Apart from that I don't want to over-engineer it but I want to prevent undefined behaviour when using Linq methods on the list. – FLav0ured Nov 26 '18 at 22:07

0 Answers0