Fix race condition #104 (#108)

Add unit test
sillsdev · Sep 5, 2023 · 3f3ff10 · 3f3ff10
1 parent 4a86bf6
commit 3f3ff10
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 9 deletions.
diff --git a/src/Serval.DataFiles/Services/DataFileService.cs b/src/Serval.DataFiles/Services/DataFileService.cs
@@ -103,19 +103,28 @@ await _deletedFiles.InsertAsync(
 
     public override async Task<bool> DeleteAsync(string id, CancellationToken cancellationToken = default)
     {
+        // return true if the deletion was successful, false if the file did not exist or was already deleted.
         await _dataAccessContext.BeginTransactionAsync(cancellationToken);
         DataFile? dataFile = await Entities.DeleteAsync(id, cancellationToken);
         if (dataFile is not null)
         {
-            await _deletedFiles.InsertAsync(
-                new DeletedFile
-                {
-                    Id = id,
-                    Filename = dataFile.Filename,
-                    DeletedAt = DateTime.UtcNow
-                },
-                cancellationToken
-            );
+            try
+            {
+                await _deletedFiles.InsertAsync(
+                    new DeletedFile
+                    {
+                        Id = id,
+                        Filename = dataFile.Filename,
+                        DeletedAt = DateTime.UtcNow
+                    },
+                    cancellationToken
+                );
+            }
+            catch (DuplicateKeyException)
+            {
+                // if it was already deleted, return false
+                return false;
+            }
         }
         await _mediator.Publish(new DataFileDeleted { DataFileId = id }, cancellationToken);
         await _dataAccessContext.CommitTransactionAsync(CancellationToken.None);

diff --git a/tests/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs b/tests/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs
@@ -115,6 +115,34 @@ public async Task DeleteAsync_DoesNotExist()
         Assert.That(deleted, Is.False);
     }
 
+    [Test]
+    public async Task DeleteAsync_Twice()
+    {
+        var env = new TestEnvironment();
+        env.DataFiles.Add(
+            new DataFile
+            {
+                Id = DATA_FILE_ID,
+                Name = "file1",
+                Filename = "file1.txt"
+            }
+        );
+        bool deleted = await env.Service.DeleteAsync(DATA_FILE_ID);
+        Assert.That(deleted, Is.True);
+        // The same file would not be added twice, but this is to check
+        env.DataFiles.Add(
+            new DataFile
+            {
+                Id = DATA_FILE_ID,
+                Name = "file1",
+                Filename = "file1.txt"
+            }
+        );
+        deleted = await env.Service.DeleteAsync(DATA_FILE_ID);
+        // The real condition is a race condition - trying to delete the same file twice (at the same time).  One should fail.
+        Assert.That(deleted, Is.False);
+    }
+
     private class TestEnvironment
     {
         public TestEnvironment()