NHibernate transaction isn't isolated

Question

I have the following code in a MVC controller, using the NHibernate framework:

    [HttpPost]
    public void FinishedExecutingTurn()
    {
        using (GameUnitOfWork unitOfWork = new GameUnitOfWork())
        {
            int currentUid = int.Parse(User.Identity.Name);
            Game game = unitOfWork.Games.GetActiveGameOfUser(currentUid);
            Player localPlayer = game.Players.First(p => p.User.Id == currentUid);
            localPlayer.FinishedExecutingTurn = true;

            if (game.Players.All(p => p.FinishedExecutingTurn))
            {
                //do some stuff
            }
            unitOfWork.Commit();
        }
    }

What GameUnitOfWork does is use a session-per-request (which is saved in the HttpContext.Current.Items) and start a transaction.

The problem I'm having is that when 2 requests arrive simultaneously, it seems like the transactions aren't happening in isolation.

It is a game with 2 players. I have a situation in which each of the players sends a request to the server at approximately the same time. The transaction is supposed to set the field FinishedExecutingTurn to true on the player who sent the request. If both players are now set to true, something should happen ("do some stuff").

If each transaction happens in isolation, to my understanding one of the transactions should happen first and set FinishedExecutingTurn on one player to true, and then the transaction in the other request should set FinishedExecutingTurn to true on the second player and enter my if statement ("do some stuff"). However, sometimes it doesn't enter the if statement at all, because FinishedExecutingTurn is initially set to false on both players, in both of the requests.

My question is, shouldn't one transaction necessarily happen first and set the field to true, and then in the other request, one of the players should already be set to true?

Answer 1

This is not NHibernate responsibility but the database one.

to my understanding one of the transactions should happen first

No. Transaction being isolated does not mean they are serialized. Under default isolation level for most cases, ReadCommitted, it just means that one transaction can not read the changes another ongoing transaction has made. Depending on the database engine, it will either read previous values (by example, Oracle does that) or be blocked instead (SQL-Server when snapshot isolation is not enabled). Transactions can still happen concurrently, even when reading the same rows of data.

So you have two players concurrently reading other player state then flagging themselves as having ended their turn. This can happen even when the database block reading modified data instead of yielding previous value, since your update can happen after the read. So both can read other player state as not having ended its turn.

You can cause the write to be blocked by choosing another isolation level, RepeatableRead. This will cause the two concurrent transactions to deadlock, one being cancelled (victim) and the other proceeded. The cancelled one should then be replayed, and since the non-victim would have either ended at that point or acquired an exclusive lock by writing the flag, the replayed transaction will then be able to read other player as having ended its turn (either immediately or by waiting other transaction end due to its exclusive lock forbidding the replayed one to put a shared lock on it, required for reading with this isolation level).

while (true)
{
    try
    {
        using (GameUnitOfWork unitOfWork = new GameUnitOfWork())
        {
            int currentUid = int.Parse(User.Identity.Name);
            Game game = unitOfWork.Games.GetActiveGameOfUser(currentUid);
            Player localPlayer = game.Players.First(p => p.User.Id == currentUid);
            localPlayer.FinishedExecutingTurn = true;

            if (game.Players.All(p => p.FinishedExecutingTurn))
            {
                //do some stuff
            }
            unitOfWork.Commit();
        }
        return;
    }
    catch (GenericADOException ex)
    {
        // SQL-Server specific code for identifying deadlocks
        // Adapt according to your database errors.
        var sqlEx = ex.InnerException as SqlException;
        if (sqlEx == null || sqlEx.Number != 1205)
            throw;
        // Deadlock, just try again by letting the loop go on (eventually
        // log it).
        // Need to acquire a new session, previous one is dead, put some
        // code here for disposing your previous contextual session and 
        // put a new one instead.
    }
}

Setting the isolation level can be done with session.BeginTransaction(IsolationLevel.Serializable). Doing that reduces the ability of serving many concurrent requests, so better do that only for cases requiring it. If you use TransactionScope, their constructor take the IsolationLevel as argument too.

You may try instead to write current player state before reading other player state (by using a session.Flush after writing, then querying the other player state). This could work for databases using shared locks for reading and exclusives lock for writing when under ReadCommitted isolation level. But there too, you will have to handle deadlocks. And it will not work for databases which do not use shared locks for reading under ReadCommitted, but yield instead the last committed value.

Note:

Maybe should you change completely your pattern instead: record each player actions, without handling "end of turn" operations in the "last" player request. Then use some background process to handle plays where all players have ended their turn.

This can be done with some queuing technology, with the flagging of player end of turn being queued. Polling database could work but is not very good.

Answer 2

After a lot of reading about concurrency in databases and locks, I finally found a solution. I simply defined this transaction with an isolation level of "RepeatableRead":

transaction = session.BeginTransaction(IsolationLevel.RepeatableRead);

This was actually one of the first solutions I tried, but I initially got deadlocks when using it. Later, when I tried using this isolation method only for this specific transaction that required it, the deadlocks seemed to disappear.

What "RepeatableRead" is supposed to achieve is locking the fetched players' rows until the first request has committed the transaction.

However, as I have no previous experience in the topic of locks, I would appreciate receiving other answers from experts in this subject.