CODEWITHSUNDEEP
c#multithreadingentity-frameworkwcf
JensB
JensB

Reputation: 6850

Avoiding same code running twice with parallel calls to WCF

I am making a small game in which users send in moves using a WCF and when all moves have been sent in the game simulates the next round.

The data is stored in an MSSQL database and accessed through EntityFramework.

The check for if all moves have been sent in and simulation should start is done at the end of each call.

There is a problem with this. If both players send in their moves within a very small window of time there is a chance both threads will check if all moves have been sent in, find that they are and start two simulations before either one gets far enough to set the status to simulating (and block any further attempts).

In order to remedy this I wrote the following check.

The general idea is this:

I need some input on if there are any loop holes in this or how this can be improved.

    private void TryToRunSimulationRound(GWDatabase db, GameModel game)
    {
        // if we simulate, this is going to be our id while doing it
        Guid simulatorGuid = Guid.NewGuid();

        // force reload of game record
        db.Entry(game).Reload();

        // is game in right state, and have all moves been received? if so kick it in to gear!
        if (game.GameState == GameState.ActiveWaitingForMoves &&  game.Players.Any(x => x.NextMoves == null || x.NextMoves.Count() == 0) == false)
        {
            // set simulation id
            game.SimulatorGuid = simulatorGuid;
            game.GameState = GameState.ActiveWaitingForSimulation;
            db.SaveChanges();

            // wait to see if someone else might be simulating
            Thread.Sleep(100);

            // get a new copy of the game to check against
            db.Entry(game).Reload();

            // if we still have same guid on the simulator then we can go ahead.. otherwise stop, someone else is running.
            // this will allow the later thread to do the simulation while the earlier thread will skip it.
            if (simulatorGuid == game.SimulatorGuid && game.GameState == GameState.ActiveWaitingForSimulation)
            {
                var s = new SimulationHandler();

                s.RunSimulation(db, game.Id);

                game.SimulatorGuid = null;

                // wait a little in the end just to make sure we dont have any slow threads just getting to the check...
                Thread.Sleep(100); 
                db.SaveChanges();
            }
            else
            {
                GeneralHelpers.AddLog(db, game, null, GameLogType.Debug, "Duplicate simulations stopped, game: " + game.Id);
            }
        }

P.S. This error took me a long time to figure out, and it wasent untill I ran a System.Threading.Tasks.Parallel.ForEach with 5000 queries that I could reproduce it every time. This case will probably never happen in the real world, but this is for a hobby project, and its fun to play around with ;)

UPDATE This is the updated code after Juan's answer. This also stops the error from happening and seems to be a cleaner solution. 

    private static readonly object _SimulationLock = new object();

    private void TryToRunSimulationRound(GWDatabase db, GameModel game)
    {
        bool runSimulation = false;
        lock (_SimulationLock) //Ensure that only 1 thread checks to run at a time.
        {
            // force reload of game record
            db.Entry(game).Reload();

            if (game.GameState == GameState.ActiveWaitingForMoves 
                && game.Players.Any(x => x.NextMoves == null || x.NextMoves.Count() == 0) == false)
            {
                game.GameState = GameState.ActiveWaitingForSimulation;
                db.SaveChanges();

                runSimulation = true;
            }
        }

        if(runSimulation){
            // get a new copy of the game to check against
            db.Entry(game).Reload();

            if (game.GameState == GameState.ActiveWaitingForSimulation)
            {
                var s = new SimulationHandler();
                s.RunSimulation(db, game.Id);
                db.SaveChanges();
            }
        }
    }

Upvotes: 2

Views: 579

Answers (1)

Juan Pablo Villaseca
Juan Pablo Villaseca

Reputation: 59

Based on your code, if both threads access at the same time on this condition:

if (game.GameState == GameState.ActiveWaitingForMoves &&  game.Players.Any(x => x.NextMoves == null || x.NextMoves.Count() == 0) == false)

Both will try and set the simulation ID, and manipulate the state concurrently, which is bad.

game.SimulatorGuid = simulatorGuid; //thread 1 is setting this, while t2 is doing that too.
game.GameState = GameState.ActiveWaitingForSimulation;
db.SaveChanges(); //which thread wins?

There is a really good pattern to prevent those race conditions, without the need to use process.sleep. It also prevents the need to fill your code with obscure conditions, if you just need an atomic access to the database (atomic ensures that all operations are done in that order, without race conditions or interference from other threads).

It could be solved by sharing a static object across threads, and using the built-in locking mechanism that enforces an atomic operation:

private static readonly object SimulationLock= new object();

and then surround your code with the locking safety precaution,

`private void AtomicRunSimulationRound(GWDatabase db, GameModel game)
{
    lock(SimulationLock) //Ensure that only 1 thread has access to the method at once
    {
        TryToRunSimulationRound(db, game);
    }
}

private void TryToRunSimulationRound(GWDatabase db, GameModel game)
{
    //Thread.sleep is not needed anymore
}`

There are more elegant solutions. Instead of waiting for the resource to be freed, I would prefer to have the gamestate check and set of the ActiveWaitingForSimulation flag done atomically, with the lock, and then returning an error that a simulation is already undergoing for the other threads that check and access that flag, since that would be an atomical operation and would be done one at a time.

Upvotes: 3

Related Questions