#816 Fix save certificate race condition

Author: justcoding121

src/Titanium.Web.Proxy/Certificates/CertificateManager.cs

@@ -412,6 +412,9 @@ private X509Certificate2 makeCertificate(string certificateName, bool isRootCert
             return certificate;
         }
 
+        private static ConcurrentDictionary<string, object> saveCertificateLocks 
+            = new ConcurrentDictionary<string, object>();
+
         /// <summary>
         ///     Create an SSL certificate
         /// </summary>
@@ -445,7 +448,21 @@ private X509Certificate2 makeCertificate(string certificateName, bool isRootCert
 
                         try
                         {
-                            certificateCache.SaveCertificate(subjectName, certificate);
+                            //acquire lock by subjectName
+                            //Async lock is not needed. Since this is a rare race-condition
+                            lock (saveCertificateLocks.GetOrAdd(subjectName, new object()))
+                            {
+                                try
+                                {
+                                    //no two tasks with same subject name should together enter here 
+                                    certificateCache.SaveCertificate(subjectName, certificate);
+                                }
+                                finally
+                                {
+                                    //save operation is complete. Free lock from memory.
+                                    saveCertificateLocks.TryRemove(subjectName, out var _);
+                                }
+                            }
                         }
                         catch (Exception e)
                         {
@@ -482,13 +499,17 @@ private X509Certificate2 makeCertificate(string certificateName, bool isRootCert
             }
 
             // handle burst requests with same certificate name
-            // by checking for existing task for same certificate name
+            // by checking for existing task for same certificate name.
+            // If two or more requests hit this block at the same time, then multiple tasks will be created,
+            // which is okay. That certificate will be created twice or more. We don't anticipate many requests for same host at the same time.
+            // This saves us from another expensive lock. The goal here is to minimize creation of multiple tasks for same certification name.
+            // Goal is not to guarantee single certificate creation, which is not needed in our case.
             if (pendingCertificateCreationTasks.TryGetValue(certificateName, out var task))
             {
                 return await task;
             }
 
-            // run certificate creation task & add it to pending tasks
+            // run certificate creation task & add it to pending tasks         
             task = Task.Run(() =>
             {
                 var result = CreateCertificate(certificateName, false);

tests/Titanium.Web.Proxy.UnitTests/CertificateManagerTests.cs

@@ -76,5 +76,31 @@ public async Task Simple_Create_Win_Certificate_Test()
             mgr.StopClearIdleCertificates();
 
         }
+
+        [TestMethod]
+        public async Task Create_Server_Certificate_Test()
+        {
+            var tasks = new List<Task>();
+
+            var mgr = new CertificateManager(null, null, false, false, false, new Lazy<ExceptionHandler>(() => (e =>
+            {
+                Debug.WriteLine(e.ToString());
+                Debug.WriteLine(e.InnerException?.ToString());
+            })).Value)
+            { CertificateEngine = CertificateEngine.BouncyCastleFast };
+
+            mgr.SaveFakeCertificates = true;
+
+            for (int i = 0; i < 500; i++)
+            {
+                tasks.AddRange(hostNames.Select(host => Task.Run(() =>
+                {
+                    var certificate = mgr.CreateServerCertificate(host);
+                    Assert.IsNotNull(certificate);
+                })));
+            }
+
+            await Task.WhenAll(tasks.ToArray());
+        }
     }
 }