commit 8d13d23305573b1370f0e465e05e222e7a8f8fc2
author Googler <noreply@google.com> Wed Apr 01 18:08:37 2020 -0400
committer David Goldman <davg@google.com> Fri Apr 17 10:06:33 2020 -0400
tree ef22a8675528cd747af029b6ee9a30d799e9d594
parent 2f58e2c12ccd54eff78633d2f89311aefaf624e1

Fix race condition when logging the runtime of a process.

`ProcessRunner.TimedProcessRunnerObserver` can be used concurrently by multiple threads. Therefore, we need to serialize access to its `pendingLogHandles` dictionary.

We use `os_unfair_lock` to serialize access because it is efficient when contention is rare, which should be the case in this situation.

PiperOrigin-RevId: 304271197

src/TulsiGenerator/ProcessRunner.swift

diff --git a/src/TulsiGenerator/ProcessRunner.swift b/src/TulsiGenerator/ProcessRunner.swift
index 8e8fc0a..9607ad1 100644
--- a/src/TulsiGenerator/ProcessRunner.swift
+++ b/src/TulsiGenerator/ProcessRunner.swift
@@ -12,9 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+import Darwin
 import Foundation
 
-
 /// Encapsulates functionality to launch and manage Processes.
 public final class ProcessRunner {
 
@@ -42,13 +42,42 @@
     private static var KVOContext: Int = 0
 
     /// Mapping between Processes and LogSessionHandles created for each.
+    ///
+    /// - Warning: Do not access directly; use `accessPendingLogHandles` instead.
+    /// - SeeAlso: `accessPendingLogHandles`
     private var pendingLogHandles = Dictionary<Process, LocalizedMessageLogger.LogSessionHandle>()
 
+    /// Lock to serialize access to `pendingLogHandles`.
+    ///
+    /// We use `os_unfair_lock` because it is efficient when contention is rare, which should be
+    /// the case in this situation.
+    private var pendingLogHandlesLock = os_unfair_lock()
+
+    /// Provides thread-safe access to `pendingLogHandles`.
+    ///
+    /// We need to access `pendingLogHandles` in a thread-safe way because
+    /// `TimedProcessRunnerObserver` can be used concurrently by multiple threads.
+    ///
+    /// - Parameter usePendingLogHandles: The critical section that reads from or writes to
+    ///                                   `pendingLogHandles`. Minimize the code in the critical
+    ///                                   section to avoid unnecessary lock contention.
+    private func accessPendingLogHandles<T>(usePendingLogHandles: (inout Dictionary<Process, LocalizedMessageLogger.LogSessionHandle>) throws -> T) rethrows -> T {
+      os_unfair_lock_lock(&pendingLogHandlesLock)
+      defer {
+        os_unfair_lock_unlock(&pendingLogHandlesLock)
+      }
+
+      return try usePendingLogHandles(&pendingLogHandles)
+    }
+
     /// Start logging the given Process with KVO to determine the time when it starts running.
     fileprivate func startLoggingProcessTime(process: Process,
                                              loggingIdentifier: String,
                                              messageLogger: LocalizedMessageLogger) {
-      self.pendingLogHandles[process] = messageLogger.startProfiling(loggingIdentifier)
+      let logSessionHandle = messageLogger.startProfiling(loggingIdentifier)
+      accessPendingLogHandles { pendingLogHandles in
+        pendingLogHandles[process] = logSessionHandle
+      }
       process.addObserver(self,
                           forKeyPath: #keyPath(Process.isRunning),
                           options: .new,
@@ -62,7 +91,9 @@
         process.removeObserver(self,
                                forKeyPath: #keyPath(Process.isRunning),
                                context: &TimedProcessRunnerObserver.KVOContext)
-        self.pendingLogHandles.removeValue(forKey: process)
+        accessPendingLogHandles { pendingLogHandles in
+          _ = pendingLogHandles.removeValue(forKey: process)
+        }
       }
     }
 
@@ -80,7 +111,9 @@
           let newValue = change?[NSKeyValueChangeKey.newKey] as? NSNumber,
           newValue.boolValue,
           let process = object as? Process {
-        pendingLogHandles[process]?.resetStartTime()
+        accessPendingLogHandles { pendingLogHandles in
+          pendingLogHandles[process]?.resetStartTime()
+        }
       }
     }
   }