Browse Source

Allow Empty EventMonitors And Try To Fix `CompositeEventMonitor` Thread-Safety (#3894)

### Issue Link :link:
#3635, #3667

### Goals :soccer:
This PR attempts to fix unreproducible `CompositeEventMonitor` crashes
in two ways:

1. Passing `[]` to the `Session` inits now creates a
`CompositeEventMonitor` that's properly empty rather than always
including an `AlamofireNotifications` monitor. This should allow anyone
seeing this crash to empty out the monitors to see if the crash is
resolved. **Technically this is a breaking change** as anyone passing
`[]` currently has notification support. This should be rare, and this
fix is important enough to justify breaking that non-obvious usage.
2. The stored monitors in the `CompositeEventMonitor` are now protected
and accessed through a lock, so should be made safer. Of course, I could
never figure out how any access there could be unsafe since it should
always be accessed on the serial internal queue, so we'll see if that
helps anything.

### Implementation Details :construction:
`Session` init updated, lock added.

### Testing Details :mag:
No tests added, test suite passes.
Jon Shier 1 year ago
parent
commit
17c7f9b031

+ 11 - 10
.github/workflows/ci.yml

@@ -310,14 +310,15 @@ jobs:
       - uses: actions/checkout@v4
       - name: ${{ matrix.image }}
         run: swift build --build-tests -c debug
-  Android:
-    name: Android
-    uses: hggz/swift-android-sdk/.github/workflows/sdks.yml@ci
-    strategy:
-      fail-fast: false
-    with:
-      target-repo: ${{ github.repository }}
-      checkout-hash: ${{ github.sha }}
+  # Disabled to find consistently updated builder.
+  # Android:
+  #   name: Android
+  #   uses: hggz/swift-android-sdk/.github/workflows/sdks.yml@ci
+  #   strategy:
+  #     fail-fast: false
+  #   with:
+  #     target-repo: ${{ github.repository }}
+  #     checkout-hash: ${{ github.sha }}
   Windows:
     name: ${{ matrix.name }}
     runs-on: windows-latest
@@ -348,9 +349,9 @@ jobs:
           swift build --build-tests -c debug -Xlinker /INCREMENTAL:NO -v
   CodeQL:
     name: Analyze with CodeQL
-    runs-on: macOS-13
+    runs-on: macOS-14
     env:
-      DEVELOPER_DIR: "/Applications/Xcode_14.3.1.app/Contents/Developer"
+      DEVELOPER_DIR: "/Applications/Xcode_15.4.app/Contents/Developer"
     timeout-minutes: 10
     steps:
       - name: Clone

+ 3 - 0
Source/Core/Notifications.swift

@@ -81,6 +81,9 @@ extension String {
 
 /// `EventMonitor` that provides Alamofire's notifications.
 public final class AlamofireNotifications: EventMonitor {
+    /// Creates an instance.
+    public init() {}
+
     public func requestDidResume(_ request: Request) {
         NotificationCenter.default.postNotification(named: Request.didResumeNotification, with: request)
     }

+ 8 - 9
Source/Core/Session.swift

@@ -61,9 +61,10 @@ open class Session {
     public let redirectHandler: RedirectHandler?
     /// `CachedResponseHandler` instance used to provide customization of cached response handling.
     public let cachedResponseHandler: CachedResponseHandler?
-    /// `CompositeEventMonitor` used to compose Alamofire's `defaultEventMonitors` and any passed `EventMonitor`s.
+    /// `CompositeEventMonitor` used to compose any passed `EventMonitor`s.
     public let eventMonitor: CompositeEventMonitor
-    /// `EventMonitor`s included in all instances. `[AlamofireNotifications()]` by default.
+    /// `EventMonitor`s included in all instances unless overwritten. `[AlamofireNotifications()]` by default.
+    @available(*, deprecated, message: "Use [AlamofireNotifications()] directly.")
     public let defaultEventMonitors: [EventMonitor] = [AlamofireNotifications()]
 
     /// Internal map between `Request`s and any `URLSessionTasks` that may be in flight for them.
@@ -103,8 +104,7 @@ open class Session {
     ///                               default.
     ///   - cachedResponseHandler:    `CachedResponseHandler` to be used by all `Request`s created by this instance.
     ///                               `nil` by default.
-    ///   - eventMonitors:            Additional `EventMonitor`s used by the instance. Alamofire always adds a
-    ///                               `AlamofireNotifications` `EventMonitor` to the array passed here. `[]` by default.
+    ///   - eventMonitors:            `EventMonitor`s used by the instance. `[AlamofireNotifications()]` by default.
     public init(session: URLSession,
                 delegate: SessionDelegate,
                 rootQueue: DispatchQueue,
@@ -115,7 +115,7 @@ open class Session {
                 serverTrustManager: ServerTrustManager? = nil,
                 redirectHandler: RedirectHandler? = nil,
                 cachedResponseHandler: CachedResponseHandler? = nil,
-                eventMonitors: [EventMonitor] = []) {
+                eventMonitors: [EventMonitor] = [AlamofireNotifications()]) {
         precondition(session.configuration.identifier == nil,
                      "Alamofire does not support background URLSessionConfigurations.")
         precondition(session.delegateQueue.underlyingQueue === rootQueue,
@@ -131,7 +131,7 @@ open class Session {
         self.serverTrustManager = serverTrustManager
         self.redirectHandler = redirectHandler
         self.cachedResponseHandler = cachedResponseHandler
-        eventMonitor = CompositeEventMonitor(monitors: defaultEventMonitors + eventMonitors)
+        eventMonitor = CompositeEventMonitor(monitors: eventMonitors)
         delegate.eventMonitor = eventMonitor
         delegate.stateProvider = self
     }
@@ -168,8 +168,7 @@ open class Session {
     ///                               default.
     ///   - cachedResponseHandler:    `CachedResponseHandler` to be used by all `Request`s created by this instance.
     ///                               `nil` by default.
-    ///   - eventMonitors:            Additional `EventMonitor`s used by the instance. Alamofire always adds a
-    ///                               `AlamofireNotifications` `EventMonitor` to the array passed here. `[]` by default.
+    ///   - eventMonitors:            `EventMonitor`s used by the instance. `[AlamofireNotifications()]` by default.
     public convenience init(configuration: URLSessionConfiguration = URLSessionConfiguration.af.default,
                             delegate: SessionDelegate = SessionDelegate(),
                             rootQueue: DispatchQueue = DispatchQueue(label: "org.alamofire.session.rootQueue"),
@@ -180,7 +179,7 @@ open class Session {
                             serverTrustManager: ServerTrustManager? = nil,
                             redirectHandler: RedirectHandler? = nil,
                             cachedResponseHandler: CachedResponseHandler? = nil,
-                            eventMonitors: [EventMonitor] = []) {
+                            eventMonitors: [EventMonitor] = [AlamofireNotifications()]) {
         precondition(configuration.identifier == nil, "Alamofire does not support background URLSessionConfigurations.")
 
         // Retarget the incoming rootQueue for safety, unless it's the main queue, which we know is safe.

+ 6 - 4
Source/Features/EventMonitor.swift

@@ -315,16 +315,18 @@ extension EventMonitor {
 public final class CompositeEventMonitor: EventMonitor {
     public let queue = DispatchQueue(label: "org.alamofire.compositeEventMonitor")
 
-    let monitors: [EventMonitor]
+    let monitors: Protected<[EventMonitor]>
 
     init(monitors: [EventMonitor]) {
-        self.monitors = monitors
+        self.monitors = Protected(monitors)
     }
 
     func performEvent(_ event: @escaping (EventMonitor) -> Void) {
         queue.async {
-            for monitor in self.monitors {
-                monitor.queue.async { event(monitor) }
+            self.monitors.read { monitors in
+                for monitor in monitors {
+                    monitor.queue.async { event(monitor) }
+                }
             }
         }
     }

+ 1 - 1
Tests/RequestInterceptorTests.swift

@@ -688,7 +688,7 @@ final class SingleRetrier: RequestInterceptor {
     }
 }
 
-extension RetryResult: Equatable {
+extension Alamofire.RetryResult: Swift.Equatable {
     public static func ==(lhs: RetryResult, rhs: RetryResult) -> Bool {
         switch (lhs, rhs) {
         case (.retry, .retry),

+ 1 - 1
Tests/ResponseSerializationTests.swift

@@ -571,7 +571,7 @@ final class URLResponseSerializerTests: BaseTestCase {
 // MARK: -
 
 // used by testThatDecodableResponseSerializerSucceedsWhenDataIsNilWithEmptyResponseConformingTypeAndEmptyResponseStatusCode
-extension Bool: EmptyResponse {
+extension Swift.Bool: Alamofire.EmptyResponse {
     public static func emptyValue() -> Bool {
         true
     }

+ 2 - 2
Tests/TestHelpers.swift

@@ -475,7 +475,7 @@ struct TestResponse: Decodable {
     let args: [String: String]
 }
 
-extension HTTPHeaders: Decodable {
+extension Alamofire.HTTPHeaders: Swift.Decodable {
     public init(from decoder: Decoder) throws {
         let container = try decoder.singleValueContainer()
 
@@ -485,7 +485,7 @@ extension HTTPHeaders: Decodable {
     }
 }
 
-extension HTTPHeader: Decodable {
+extension Alamofire.HTTPHeader: Swift.Decodable {
     enum CodingKeys: String, CodingKey {
         case name, value
     }

+ 1 - 1
Tests/WebSocketTests.swift

@@ -701,7 +701,7 @@ extension WebSocketRequest {
 }
 
 @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
-extension URLSessionWebSocketTask.Message: Equatable {
+extension Foundation.URLSessionWebSocketTask.Message: Swift.Equatable {
     public static func ==(lhs: URLSessionWebSocketTask.Message, rhs: URLSessionWebSocketTask.Message) -> Bool {
         switch (lhs, rhs) {
         case let (.string(left), .string(right)):