How to change a value outside, when inside a Dispatch Semaphore (Edited)-CodePudding

i have a question, and I will tell you as clear as possible:

I need to get an object to my func, create a variable version of it, change some property values one by one, and use new version of it for saving to the cloud. My problem is, when I declare my variable, if I change property values inside my Dispatch Semaphore, the variable outside not changing somehow, there is some problem that I could not understand. Here is the code :

func savePage(model: PageModel, savingHandler: @escaping (Bool) -> Void) {

// some  code .....

var page = model // (1) I created a variable from function arg

let newQueue = DispatchQueue(label: "image upload queue")
let semaphore = DispatchSemaphore(value: 0)

newQueue.async {
    if let picURL1 = model.picURL1 {
        self.saveImagesToFireBaseStorage(pictureURL: picURL1) { urlString in
            let url = urlString   // (2) urlString value true and exist
            page.picURL1 = url    // (3) modified the new created "page" object
            print(page.picURL1!)  // (4) it works, object prints modified
        }
    }
    semaphore.signal()
}

semaphore.wait()

newQueue.async {
    if let picURL2 = model.picURL2 {
        self.saveImagesToFireBaseStorage(pictureURL: picURL2) { urlString in
            let url = urlString
            page.picURL2 = url
        }
    }
    semaphore.signal()
}

semaphore.wait()

print(page.picURL1!) //(5) "page" object has the old value?


newQueue.async {
    
    print(page.picURL1!) //(6) "page" object has the old value?
    
    do {
        try pageDocumentRef.setData(from: page)
        savingHandler(true)
    } catch let error {
        print("Error writing city to Firestore: \(error)")
    }
    semaphore.signal()
} 
semaphore.wait()

}

I should upload some pictures to the cloud and get their urls, so I can create updated version of the object and save onto the old version on the cloud. But "page" object doesn't change somehow. When inside the semaphore, it prints right value, when outside, or inside another async semaphore block, it prints old value. I am new to the concurrency and could not find a way.

What I tried before :

Using Operation Queue and adding blocks as dependencies.
Creating queue as DispatchQueue.global()

What is the thing I am missing here?

Edit : I added semaphore.wait() after second async call. It actually was in my code but I accidentally deleted it while pasting to the question, thanks to Chip Jarred for pointing it.

CodePudding user response：

Let's look at your first async call:

newQueue.async {
    if let picURL1 = model.picURL1 {
        self.saveImagesToFireBaseStorage(pictureURL: picURL1) { urlString in
            let url = urlString   // (2) urlString value true and exist
            page.picURL1 = url    // (3) modified the new created "page" object
            print(page.picURL1!)  // (4) it works, object prints modified
        }
    }
    semaphore.signal()
}

I would guess that the inner closure, that is the one passed to saveImagesToFireBaseStorage, is also called asynchronously. If I'm right about that, then saveImagesToFireBaseStorage returns almost immediately, executes the signal, but the inner closure has not run yet, so the new value isn't yet set. Then after some latency, the inner closure is finally called, but that's after your "outer" code that depends on page.picURL1 has already been run, so page.picURL1 ends up being set afterwards.

So you need to call signal in the inner closure, but you still have to handle the case where the inner closure isn't called. I'm thinking something like this:

newQueue.async {
    if let picURL1 = model.picURL1 {
        self.saveImagesToFireBaseStorage(pictureURL: picURL1) { urlString in
            let url = urlString 
            page.picURL1 = url    
            print(page.picURL1!)
            semaphore.signal() // <--- ADDED THIS
        }
        /*
        If saveImagesToFireBaseStorage might not call the closure,
        such as on error, you need to detect that and call signal here
        in the case as well, or perhaps in some closure that's called in 
        the failure case.  That depends on your design.
        */
    }
    else { semaphore.signal() } // <--- MOVED INTO `else` block
}

Your second async would need to be modified similarly.

I notice that you're not calling wait after the second async, the one that sets page.picURL2. So you have 2 calls to wait, but 3 calls to signal. That wouldn't affect whether page.picURL1 is set properly in the first async, but it does mean that semaphore will have unbalanced waits and signals at the end of your code example, and the blocking behavior of wait after the third async may not be as you expect it to be.

If it's an option for your project, refactoring to use async and await keywords would resolve the problem in a way that's easier to maintain, because it would remove the need for the semaphore entirely.

Also, if my premise that saveImagesToFireBaseStorage is called asynchronously is correct, you don't really need the async calls at all, unless there is more code in their closures that isn't shown.

Update

In comments, it was revealed the using the solution above caused the app to "freeze". This suggests that saveImagesToFireBaseStorage calls its completion handler on the same queue that savePage(model:savingHandler) is called on, and it's almost certainly DispatchQueue.main. The problem is that DispatchQueue.main is a serial queue (as is newQueue), which means it won't execute any tasks until the next iteration of its run loop, but it never gets to do that, because it calls semaphore.wait(), which blocks waiting for the completion handler for saveImagesToFireBaseStorage to call semaphore.signal. By waiting it prevents the very thing its waiting for from ever executing.

You say in comments that using async/await solved the problem. That's probably the cleanest way, for lots of reasons, not the least of which is that you get compile time checks for a lot of potential problems.

In the mean time, I came up with this solution using DispatchSemaphore. I'll put it here, in case it helps someone.

First I moved the creation of newQueue outside of savePage. Creating a dispatch queue is kind of heavy-weight operation, so you should create whatever queues you need once, and then reuse them. I assume that it's a global variable or instance property of whatever object owns savePage.

The second thing is that savePage doesn't block anymore, but we still want the sequential behavior, preferably without going to completion handler hell (deeply nested completion handlers).

I refactored the code that calls saveImagesToFireBaseStorage into a local function, and made it behave synchronously by using a DispatchSemaphore to block until it's completion handler is called, but only in that local function. I do create the DispatchSemaphore outside of that function so that I can reuse the same instance for both invocations, but I treat it as though it were a local variable in the nested function.

I also have to use a time-out for the wait, because I don't know if I can assume that the completion handler for saveImagesToFireBaseStorage will always be called. Are there failure conditions in which it wouldn't be? The time-out value is almost certainly wrong, and should be considered a place-holder for the real value. You'd need to determine the maximum latency you want to allow based on your knowledge of your app and its working environment (servers, networks, etc...).

The local function uses a key path to allow setting differently named properties of PageModel (picURL1 vs picURL2), while still consolidating the duplicated code.

Here's the refactored savePage code:

func savePage(model: PageModel, savingHandler: @escaping (Bool) -> Void)
{
    // some  code .....
    
    var page = model
    
    let saveImageDone = DispatchSemaphore(value: 0)
    let waitTimeOut = DispatchTimeInterval.microseconds(500)

    func saveModelImageToFireBaseStorage(
        from urlPath: WritableKeyPath<PageModel, String?>)
    {
        if let picURL = model[keyPath: urlPath]
        {
            saveImagesToFireBaseStorage(pictureURL: picURL)
            {
                page[keyPath: urlPath] = $0
                print("page.\(urlPath) = \(page[keyPath: urlPath]!)")
                saveImageDone.signal()
            }
            
            if .timedOut == saveImageDone.wait(timeout: .now()   waitTimeOut) {
                print("saveImagesToFireBaseStorage timed out!")
            }
        }
    }
    
    newQueue.async
    {
        saveModelImageToFireBaseStorage(from: \.picURL1)
        saveModelImageToFireBaseStorage(from: \.picURL2)
        print(page.picURL1!)
        
        do {
            try self.pageDocumentRef.setData(from: page)
            
            // Assume handler might do UI stuff, so it needs to execute
            // on main
            
            DispatchQueue.main.async { savingHandler(true) }
        } catch let error {
            print("Error writing city to Firestore: \(error)")
            
            // Should savingHandler(false) be called here?
        }
    }
}

It's important to note that savePage does not block the thread that's called on, which I believe to be DispatchQueue.main. I assume that any code that is sequentially called after a call to savePage, if any, does not depend on the results of calling savePage. Any that does depend on it should be in its savingHandler.

And speaking of savingHandler, I have to assume that it might update the UI, and since the point where it would be called is in a closure for newQueue.async it has to be explicitly called on DispatchQueue.main, so I do that.