EXC_BAD_ACCESS KERN_INVALID_ADDRESS crash in addOperation of OperationQueue

Question

I have asynchronous operation implementation like this:

class AsyncOperation: Operation {
  private enum State: String {
    case ready, executing, finished

    fileprivate var keyPath: String {
      return "is\(rawValue.capitalized)"
    }
  }
  
  private let stateAccessQueue = DispatchQueue(label: "com.beamo.asyncoperation.state")

  private var unsafeState = State.ready
  
  private var state: State {
    get {
      return stateAccessQueue.sync {
        return unsafeState
      }
    }
    set {
      willChangeValue(forKey: newValue.keyPath)
      stateAccessQueue.sync(flags: [.barrier]) {
        self.unsafeState = newValue
      }
      didChangeValue(forKey: newValue.keyPath)
    }
  }

  override var isReady: Bool {
    return super.isReady && state == .ready
  }

  override var isExecuting: Bool {
    return state == .executing
  }

  override var isFinished: Bool {
    return state == .finished
  }

  override var isAsynchronous: Bool {
    return true
  }
 
  override func start() {
    if isCancelled {
      if !isFinished {
        finish()
      }
      return
    }

    state = .executing
    main()
  }
  
  public final func finish() {
    state = .finished
  }
}

App will download multiple resources with API. Implementation of downloader is managed by operation queue and custom asynchronous operation. Here is implementation of asynchronous downloader operation:

final class DownloaderOperation: AsyncOperation {
  private let downloadTaskAccessQueue = DispatchQueue(label: "com.download.task.access.queue")
  
  private var downloadTask: URLSessionDownloadTask?
  private var threadSafeDownloadTask: URLSessionDownloadTask? {
    get {
      return downloadTaskAccessQueue.sync {
        return downloadTask
      }
    }
    set {
      downloadTaskAccessQueue.sync(flags: [.barrier]) {
        self.downloadTask = newValue
      }
    }
  }
  
  private let session: URLSession
  let download: Download
  let progressHandler: DownloadProgressHandler
  init(session: URLSession,
       download: Download,
       progressHandler: @escaping DownloadProgressHandler) {
    self.session = session
    self.download = download
    self.progressHandler = progressHandler
  }
  
  override func main() {
    let bodyModel = DownloaderRequestBody(fileUrl: download.fileUrl.absoluteString)
    let bodyData = (try? JSONEncoder().encode(bodyModel)) ?? Data()
    
 
    var request = URLRequest(url: URL(string: "api url here")!)
    request.httpMethod = "POST"
    request.httpBody = bodyData
    let task = session.downloadTask(with: request)
    task.countOfBytesClientExpectsToSend = 512
    task.countOfBytesClientExpectsToSend = 1 * 1024 * 1024 * 1024 // 1GB
    
    task.resume()
    threadSafeDownloadTask = task
    DispatchQueue.main.async {
      self.download.progress = 0
      self.download.status = .inprogress
      self.progressHandler(self.model, 0)
    }
  }
  
  override func cancel() {
    super.cancel()
    
    threadSafeDownloadTask?.cancel()
  }
}

And downloader implementation is here:

final class MultipleURLSessionDownloader: NSObject {
  private(set) var unsafeOperations: [Download: DownloaderOperation] = [:]
  private(set) var progressHandlers: [Download: DownloadProgressHandler] = [:]
  private(set) var completionHandlers: [Download: DownloadCompletionHandler] = [:]
  
  private let operationsAccessQueue = DispatchQueue(label: "com.multiple.downloader.operations")
  
  private(set) var operations: [Download: DownloaderOperation] {
    get {
      return operationsAccessQueue.sync {
        return unsafeOperations
      }
    }
    set {
      operationsAccessQueue.sync(flags: [.barrier]) {
        self.unsafeOperations = newValue
      }
    }
  }
  
  private lazy var downloadOperationQueue: OperationQueue = {
    var queue = OperationQueue()
    queue.name = "com.multiple.downloader.queue"
    queue.maxConcurrentOperationCount = 2
    return queue
  }()
  
  private(set) var urlSession = URLSession.shared
  init(configuration: URLSessionConfiguration = URLSessionConfiguration.background(withIdentifier: "com.multiple.downloader.session")) {
    super.init()
    configuration.isDiscretionary = false
    configuration.sessionSendsLaunchEvents = true
    configuration.timeoutIntervalForResource = 120
    configuration.timeoutIntervalForRequest  = 10
    self.urlSession = URLSession(configuration: configuration, delegate: self, delegateQueue: nil)
  }
  
  deinit {
    debugPrint("[MultipleURLSessionDownloader] deinit")
  }
  
  func startDownload(downloads: [Download],
                     progressHandler: @escaping DownloadProgressHandler,
                     completionHandler: @escaping DownloadCompletionHandler) {
    for download in downloads {
      startDownload(
        download: download,
        progressHandler: progressHandler,
        completionHandler: completionHandler
      )
    }
  }
  
  func cancelDownload(download: Download) {
    cancelOperation(download: download)
  }
  
  func cancelAllDownloads() {
    for download in operations.keys {
      cancelOperation(download: download)
    }
    urlSession.getTasksWithCompletionHandler { dataTasks, uploadTasks, downloadTasks  in
      dataTasks.forEach {
        $0.cancel()
      }
      uploadTasks.forEach {
        $0.cancel()
      }
      downloadTasks.forEach {
        $0.cancel()
      }
    }
  }
  
  private func startDownload(download: Download,
                             progressHandler: @escaping DownloadProgressHandler,
                             completionHandler: @escaping DownloadCompletionHandler) {
    download.status = .default
    
    progressHandlers[download] = progressHandler
    completionHandlers[download] = completionHandler
    
    if let operation = operations[download] {
      if operation.isExecuting,
         let inProgressDownload = operations.keys.first(where: { $0.id == download.id }) {
        progressHandlers[download]?(inProgressDownload, inProgressDownload.progress ?? 0)
      }
    } else {
      let operation = DownloaderOperation(
        session: urlSession,
        download: download) {[weak self] (progressDownload, progress) in
          self?.progressHandlers[progressDownload]?(progressDownload, progress)
      }
      operations[download] = operation
      
      downloadOperationQueue.addOperation(operation)
    }
  }
  
  private func cancelOperation(download: Download) {
    download.status = .cancelled
    operations[download]?.cancel()
    callCompletion(download: download, error: nil)
  }
  
  private func callCompletion(download: Download,
                              error: DownloadError?) {
    operations[download]?.finish()
    operations[download] = nil
    progressHandlers[download] = nil
    download.progress = nil
    let handler = completionHandlers[download]
    completionHandlers[download] = nil
    handler?(download, error)
  }

}

There is 2 cancel method:

1. cancelDownload(download: Download)
2. cancelAllDownloads()

If download is canceled and tried to download multiple times app is crashed and crash log is here:

If download is cancelledAll and tried to download multiple times app is crashed and crash log is here:

And the strange thing here is if I open app by running from Xcode the crash is not happened. This is happening only when if I open app from device without running by Xcode.

For now I fixed replecaing operation queue with dispatch queue like this: downloadOperationQueue.addOperation(operation)

downloadDispatchQueue.async {
    operation.start()
  }

And this is working fine without any crash.

I think crash is happening on addOperation method of OperationQueue, but I don't know the reason.

Answer 1

It does not make much sense to use operations in conjunction with a background URLSession. The whole idea of background URLSession is network requests proceed if the user leaves the app, and even if the app has been terminated in the course of its normal lifecycle (e.g., jettisoned due to memory pressure). But an operation queue cannot survive the termination of an app. If you really want to use operations with background session, you will need to figure out how you will handle network requests that are still underway, but for which there is no longer any operation to represent that request.

---

Let us set that aside and look at the operation and diagnose the crash.

A critical issue is the isFinished KVO when the operation is canceled. The documentation is very clear that if the operation has started and is subsequently canceled, you must perform the isFinished KVO notifications. If the task not yet started. If you do that, you can receive a warning:

went isFinished=YES without being started by the queue it is in

That would seem to suggest that one should not issue isFinished KVO notifications if an unstarted operation is canceled. But, note you absolutely must set the underlying state to finished, or else the unstarted canceled operations will be retained by the operation queue.

But what is worse, that performing the isFinished KVO notifications for unstarted operations it can lead to NSKeyValueDidChangeWithPerThreadPendingNotifications in the stack trace, just like you showed in your question. Here is my stack trace from my crash:

So, all of that said, I discovered two work-arounds:

1. Use locks for synchronization rather than GCD. They are more performant, anyway, and in my experiments, avoid the crash. (This is an unsatisfying solution because it is unclear as to whether it really solved the root problem, or just moved the goal-posts sufficiently, such that the crash no longer manifests itself.)

2. Alternatively, when you set the state to .finished, only issue the isFinished KVO if the operation was currently running. (This is also a deeply unsatisfying solution, as it is contemplated nowhere in Apple’s documentation. But it silences the above warning and eliminates the crash.)

   For example:

   func finish() {
       if isExecuting {
           state = .finished                    // change state with KVO
       } else {
           synchronized { _state = .finished }  // change state without KVO
       }
   }
   

   Basically, that sets the state to finished either way, with KVO notifications if the operation is executing, and without if not yet started when it is canceled.

---

So, you might end up with:

open class AsynchronousOperation: Operation {
    override open var isReady: Bool        { super.isReady && state == .ready }
    override open var isExecuting: Bool    { state == .executing }
    override open var isFinished: Bool     { state == .finished }
    override open var isAsynchronous: Bool { true }

    private let lock = NSLock()

    private var _state: State = .ready

    private var state: State {
        get { synchronized { _state } }

        set {
            let oldValue = state

            guard oldValue != .finished else { return }

            willChangeValue(forKey: oldValue.keyPath)
            willChangeValue(forKey: newValue.keyPath)

            synchronized { _state = newValue }

            didChangeValue(forKey: newValue.keyPath)
            didChangeValue(forKey: oldValue.keyPath)
        }
    }

    override open func start() {
        if isCancelled {
            finish()
            return
        }

        state = .executing
        main()
    }

    open func finish() {
        state = .finished
    }
}

// MARK: - State

private extension AsynchronousOperation {
    enum State: String {
        case ready, executing, finished

        fileprivate var keyPath: String {
            return "is\(rawValue.capitalized)"
        }
    }
}

// MARK: - Private utility methods

private extension AsynchronousOperation {
    func synchronized<T>(block: () throws -> T) rethrows -> T {
        lock.lock()
        defer { lock.unlock() }
        return try block()
    }
}

A few notes on the above:

• If we refer to Listing 2-7 the old Concurrency Programming Guide: Operations, they illustrate (in Objective-C) the correct KVO notifications that must take place when we, for example, transition from “executing” to “finished”:

  - (void)completeOperation {
      [self willChangeValueForKey:@"isFinished"];
      [self willChangeValueForKey:@"isExecuting"];
  
      executing = NO;
      finished = YES;
  
      [self didChangeValueForKey:@"isExecuting"];
      [self didChangeValueForKey:@"isFinished"];
  }
  

  So, as we transition from executing to finished, we have to perform the notification for both isExecuting and isFinished. But your state setter is only performing the KVO for the newValue (thus, only isFinished, neglecting to perform the isExecuting-related KVO notifications). This is likely unrelated to your problem at hand, but is important, nonetheless.

• If you were to synchronize with GCD serial queue, the barrier becomes redundant. This lingering barrier was probably a legacy of a previous rendition using a reader-writer pattern. IMHO, the transition to a serial queue was prudent (as the reader-writer just introduces more problems than its negligible performance difference warrants). But if we eliminate the concurrent queue, we should also remove the redundant barrier. Or just eliminate GCD altogether as shown above.

• I guard against state changes after the operation is already finished. This is a bit of defensive-programming adapted from Apple’s “Advanced NSOperations” implementation (which, admittedly, is no longer available on Apple’s site).

• I would not recommend making finish a final function. While unlikely, it is possible that a subclass might want to add functionality. So I removed final.

• I moved the GCD synchronization code into its own method, synchronized, so that I could easily switch between different synchronization mechanisms.

• Given that Operation is an open class, I did the same here.