Prevent concurrent selection of same next calculated value

Question

I have a table called SerialNumber in my database that looks like this:

[Id] [bigint] IDENTITY(1,1) NOT NULL,
[WorkOrderId] [int] NOT NULL,
[SerialValue] [bigint] NOT NULL,
[SerialStatusId] [int] NOT NULL

Where in my application I always need to get the MIN SerialValuewith a SerialStatusId of 1 for a given Work Order (WorkOrderId).
i.e if a client have selected a certain serial number it should immediately change to SerialStatusId =2 so that the next client will not select the same value.
The application is installed on multiple stations which may access the Database simultaneously.
I tried a few approaches to solve this but without success:

update SerialNumber
set SerialStatusId = 2
output inserted.SerialValue
where WorkOrderId = @wid AND 
SerialValue = (select MIN(SerialValue) FROM SerialNumber where SerialStatusId = 1)

AND

declare @val bigint;
set @val = (select MIN(SerialNumber.SerialValue) from SerialNumber where SerialStatusId = 1
and WorkOrderId = @wid);

select SerialNumber.SerialValue from SerialNumber where SerialValue = @val;
update SerialNumber set SerialStatusId = 2 where SerialValue = @val;

And the way I tested to see that don't select identical values is:

    public void Test_GetNext()
    {
        Task t1 = Task.Factory.StartNew(() => { getSerial(); });
        Task t2 = Task.Factory.StartNew(() => { getSerial(); });
        Task t3 = Task.Factory.StartNew(() => { getSerial(); });
        Task t4 = Task.Factory.StartNew(() => { getSerial(); });
        Task t5 = Task.Factory.StartNew(() => { getSerial(); });

        Task.WaitAll(t1, t2, t3, t4, t5);


        var duplicateKeys = serials.GroupBy(x => x)
                    .Where(group => group.Count() > 1)
                    .Select(group => group.Key).ToList();
    }

    private List<long> serials = new List<long>();
    private void getSerial()
    {
        object locker = new object();
        SerialNumberRepository repo = new SerialNumberRepository();
        while (serials.Count < 200)
        {
            lock (locker)
            {
                serials.Add(repo.GetNextSerialWithStatusNone(workOrderId));
            }
        }
    }

All the tests I've made so far gave ~70 duplicate values from a table that contains 200 serials. How can I improve my SQL statement so it won't return duplicate values?
UPDATE
Tried to implement what @Chris suggeted but still getting the amount of duplicated:

SET XACT_ABORT, NOCOUNT ON
DECLARE @starttrancount int;
BEGIN TRY
SELECT @starttrancount = @@TRANCOUNT
IF @starttrancount = 0 BEGIN TRANSACTION
update SerialNumber
set SerialStatusId = 2
output deleted.SerialValue
where WorkOrderId = 35 AND
SerialValue = (select MIN(SerialValue) FROM SerialNumber where SerialStatusId = 1)
IF @starttrancount = 0
COMMIT TRANSACTION
ELSE
EXEC sp_releaseapplock 'get_code';
END TRY
BEGIN CATCH
IF XACT_STATE() <> 0 AND @starttrancount = 0
ROLLBACK TRANSACTION
RAISERROR (50000,-1,-1, 'mysp_CreateCustomer')
END CATCH

Answer 1

The reason why you are getting duplicates is because there is a race condition between the time the previous value is read, and the new value is updated, into SerialNumber. If two concurrent connections (e.g. two different threads) read the same value simultaneously, they will both derive the same new SerialNumber. The second connection will redundantly also update the status again.

Updated Solution - previous one caused deadlocks

To solve this using your current design, when reading the next value of the you'll need to lock the row in the table up front to prevent other parallel readers.

CREATE PROC dbo.GetNextSerialLocked(@wid INT)
AS 
BEGIN
    BEGIN TRAN
        DECLARE @NextAvailableSerial BIGINT;
        SELECT @NextAvailableSerial = MIN(SerialValue) 
           FROM SerialNumber WITH (UPDLOCK, HOLDLOCK) where SerialStatusId = 1;

        UPDATE SerialNumber
        SET SerialStatusId = 2
        OUTPUT inserted.SerialValue
        WHERE WorkOrderId = @wid AND SerialValue = @NextAvailableSerial;
    COMMIT TRAN
END
GO

Since the acquire + update is now split across multiple statements, we do need a transaction for to control the lifespan of the HOLDLOCK.

You could also increase the transaction isolation level to SERIALIZABLE - although this won't prevent the parallel reads, it will however cause the second writer to deadlock when it also tries to write to the table, given that the value read has now been changed.

You might also consider alternative designs to avoid obtaining data in this way, e.g. if the serial numbers should increment, consider an IDENTITY.

Also note that your C# code locking has no effect, since each Task will have an independent locker object and there will never be contention for the same lock.

    object locker = new object(); // <-- Needs to be shared between Tasks
    SerialNumberRepository repo = new SerialNumberRepository();
    while (serials.Count < 200)
    {
        lock (locker) <-- as is, no effect.

Also, when adding the serials, you'll need a memory barrier around the serials collection. Simplest is probably just to change it to a concurrent collection of sorts:

private ConcurrentBag<long> serials = new ConcurrentBag<long>();

If you wanted to serialize access from C#, you would need to use a shared static object instance across all tasks / threads (and obviously serializing from code would require that all access to the Serial Number comes from the same process). Serializing in the database is much more scalable since you can also have multiple concurrent readers for different WorkOrderId

• SqlFiddle of the Sql Code here
• GitHub Gist of the C# code

Answer 2

Take a look at the answer to this question: TSQL mutual exclusive access in a stored procedure.

You need to ensure mutual exclusive access to the table, you're currently just relying on luck (which, as you've seen isn't working).