0

If uploadFailed(for id: String), uploadSuccess() and updateOnStart(_ id: String) are called from the same thread (Thread 1 Main Thread) I understand that we wouldn't need a synchronized Queue. What if the the functions are called from different threads for each uploads. Where do I ensure there is synchronization? Will it be both the uploads and state or just the state?

enum FlowState {
 case started(uploads: [String])
 case submitted
 case failed
}

class Session {
 var state: FlowState
 let syncQueue: DispatchQueue = .init(label: "Image Upload Sync Queue",
                                      qos: .userInitiated,
                                      attributes: [],
                                      autoreleaseFrequency: .workItem)

 init(state: FlowState) {
   self.state = state
 }

 mutating func updateOnStart(_ id: String) {
  guard case .started(var uploads) = state else {
    return
  }

  uploads.append(id)

  state = .started(uploads)
}

  mutating func uploadFailed(for id: String) {
   guard case .started(var uploads) = state else {
    return
   }

   uploads.removeAll { $0 == id }

   if uploads.isEmpty {
      state = .failed
   } else {
      state = .started(uploads)
   }
 }

 mutating func uploadSuccess() {
  state = .submitted
 }

}

Do we synchronize both the uploads array operation and state like below?

syncQueue.sync {
  uploads.append(id)

  state = .started(uploads)
}


syncQueue.sync {
  uploads.removeAll { $0 == id }

   if uploads.isEmpty {
      state = .failed
   } else {
      state = .started(uploads)
   }
}

OR

syncQueue.sync {
  state = .started(uploads)
}

syncQueue.sync {
  if uploads.isEmpty {
    state = .failed
  } else {
    state = .started(uploads)
  }
}

A network call's completion handler can update Session's state property. The user for example selects 10 images and uploads it. Upon completion it could be a failure or a success. For every image that we upload, we cache the resouce id and we remove it if uploads fails. When all the image upload fails, we update the status .failed. We just care about one image being uploaded. When a single image is uploaded, we update the status to .submitted

DesperateLearner
  • 1,115
  • 3
  • 19
  • 45
  • What do you mean by, "Will it be both the uploads and state or just the state?"? – Joshua Wolff Mar 24 '20 at 04:50
  • 1
    Can you give background on your specific use case? What are you trying to do? – Joshua Wolff Mar 24 '20 at 04:51
  • 1
    @JoshWolff I updated the question with more information – DesperateLearner Mar 24 '20 at 05:28
  • "What if the the functions are called from different threads for each uploads" Maybe this seems like a superficial reaction but my first thought would be that it's your job to ensure that that doesn't happen...? – matt Mar 24 '20 at 17:41
  • The callbacks are yet to be created and someone else is going to work on the uploads with simultaneous uploads asynchronously. It's just a fallback – DesperateLearner Mar 24 '20 at 19:49
  • Needless to say, we wouldn’t use `mutating` with a `class` type. I’m assuming this is a remnant of some earlier `struct` experiments... – Rob Mar 24 '20 at 21:09

1 Answers1

1

Using synchronization to ensure thread-safe interaction with FlowState is perfectly valid.

A few observations:

  1. You present two alternatives:

    syncQueue.sync {
        uploads.append(id)
    
        self.state = .started(uploads)
    }
    

    Or

    syncQueue.sync {
        state = .started(uploads)
    }
    

    Neither of these are quite right. What if thread 1 and thread 2 are calling this routine at the same time? Consider:

    • Thread 1 retrieves uploads
    • Thread 2 retrieves the same uploads;
    • Thread 1 then enters its sync block and appends a record to its local copy, saves it, and leaves its sync closure;
    • Thread 2 then enters its sync block and appends a different record to its own local copy (without the record added by thread 1), saves it, and leaves its sync closure.
       

    In this scenario, you’ll lose what thread 1 appended.

    So, you need to employ a third, broader synchronization mechanism, wrap the whole process of retrieving, appending, and storing uploads within a synchronization mechanism:

    syncQueue.sync {
        guard case .started(var uploads) = self.state else {
            return
        }
    
        uploads.append(id)
    
        state = .started(uploads)
    }
    
  2. If you haven’t, I’d suggest turning on the thread sanitizer (TSAN) during your development and testing. It might help you catch these sorts of issues.

  3. You’re not showing any additional reads of state, but if do have any, make sure you synchronize your reads, too. All interaction with state must be synchronized.

  4. A subtle issue: If you’re going to synchronize your access to state, make sure you make it private so that no external code has access to it (or else you’re potentially thwarting your attempt to ensure thread-safe interaction). You need to wrap all reads and writes within your synchronization mechanism.

    You probably should make the sync queue private, too, as there’s no need to expose that, either.

Rob
  • 415,655
  • 72
  • 787
  • 1,044
  • Thank you Rob. That was really helpful. `public private(set) var state: FlowState` state is only readable publicly – DesperateLearner Mar 25 '20 at 01:31
  • 1
    No offense, but you can’t have other threads reading this while you might be writing. You do not want to expose any unsynchronized access, even if only reads. – Rob Mar 25 '20 at 04:50