12
class SomeViewController: UIViewController {
    let semaphore = DispatchSemaphore(value: 1)

    deinit {
        semaphore.signal() // just in case?
    }

    func someLongAsyncTask() {
        semaphore.wait()
        ...
        semaphore.signal() // called much later
    }
}

If I tell a semaphore to wait and then deinitialize the view controller that owns it before the semaphore was ever told to signal, the app crashes with an Thread 1: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) error. However, if I simply call semaphore.signal() in the deinit method of the view controller, disaster averted. However, if the async function returns before deinit is called and the view controller is deinitialized, then signal() is called twice, which doesn't seem problematic. But is it safe and/or wise to do this?

Rob
  • 415,655
  • 72
  • 787
  • 1,044
lurning too koad
  • 2,698
  • 1
  • 17
  • 47
  • 2
    Slight reframe: is there a specific reason you're using semaphores over another tool, especially long-lived semaphores? Semaphores can be pretty tricky, as you see here, and can also be impactful on performance — specifically, semaphores can end up defeating tricks which help avoid priority inversions (Dispatch can help bump queue priority when you're using queues, but can't do anything for you around something as low level as a semaphore) – Itai Ferber Dec 23 '21 at 02:43
  • @ItaiFerber I really just want to understand the semaphore and how to use it safely before I decide if I want to use it or not. I have a function with async work that itself spawns more async work and I'd prefer it if this function would finish fully before running again (since it could be called in rapid succession from a live database listener). If I can safely use a semaphore here then I will but if I can't then I won't. – lurning too koad Dec 23 '21 at 03:10
  • 2
    @kidcoder - FWIW, your code snippet is not enough to produce the problem you describe. Generally, we see this problem if (a) you have a path of execution where you might not hit the `semaphore.signal()` call; or (b) you are using weak references and really have a `self?.semaphore.signal()` or something equivalent. – Rob Dec 23 '21 at 07:37

1 Answers1

9

You have stumbled into a feature/bug in DispatchSemaphore. If you look at the stack trace and jump to the top of the stack, you'll see assembly with a message:

BUG IN CLIENT OF LIBDISPATCH: Semaphore object deallocated while in use

E.g.,

enter image description here

This is because DispatchSemaphore checks to see whether the semaphore’s associated value is less at deinit than at init, and if so, it fails. In short, if the value is less, libDispatch concludes that the semaphore is still being used.

This may appear to be overly conservative, as this generally happens if the client was sloppy, not because there is necessarily some serious problem. And it would be nice if it issued a meaningful exception message rather forcing us to dig through stack traces. But that is how libDispatch works, and we have to live with it.

All of that having been said, there are three possible solutions:

  1. You obviously have a path of execution where you are waiting and not reaching the signal before the object is being deallocated. Change the code so that this cannot happen and your problem goes away.

  2. While you should just make sure that wait and signal calls are balanced (fixing the source of the problem), you can use the approach in your question (to address the symptoms of the problem). But that deinit approach solves the problem through the use of non-local reasoning. If you change the initialization, so the value is, for example, five, you or some future programmer have to remember to also go to deinit and insert four more signal calls.

    The other approach is to instantiate the semaphore with a value of zero and then, during initialization, just signal enough times to get the value up to where you want it. Then you won’t have this problem. This keeps the resolution of the problem localized in initialization rather than trying to have to remember to adjust deinit every time you change the non-zero value during initialization.

    See https://lists.apple.com/archives/cocoa-dev/2014/Apr/msg00483.html.

  3. Itai enumerated a number of reasons that one should not use semaphores at all. There are lots of other reasons, too:

    • Semaphores are incompatible with new Swift concurrency system (see Swift concurrency: Behind the scenes);
    • Semaphores can also easily introduce deadlocks if not precise in one’s code;
    • Semaphores are generally antithetical to cancellable asynchronous routines; etc.

    Nowadays, semaphores are almost always the wrong solution. If you tell us what problem you are trying to solve with the semaphore, we might be able to recommend other, better, solutions.


You said:

However, if the async function returns before deinit is called and the view controller is deinitialized, then signal() is called twice, which doesn't seem problematic. But is it safe and/or wise to do this?

Technically speaking, over-signaling does not introduce new problems, so you don't really have to worry about that. But this “just in case” over-signaling does have a hint of code smell about it. It tells you that you have cases where you are waiting but never reaching signaling, which suggests a logic error (see point 1 above).

Rob
  • 415,655
  • 72
  • 787
  • 1,044
  • I think you have convinced me to avoid the use of them, in this use case anyway. If I'm working with a permanent view controller, for example, that lives the life of the app, and I can safely use one where I really want/need one, I will reconsider using them then. But can you expand just a bit on what you mean when you say semaphores are incompatible with the new Swift concurrency system? – lurning too koad Dec 23 '21 at 17:29
  • Thank you for that. I will watch it carefully. So if semaphores are on their way out, is there a tool to prevent an asynchronous task from running in parallel? In other words, if we should not block a thread from executing a task because another instance of that task is already running, is the idea just to get really creative and structure things in a way that don't matter if this task runs in parallel? – lurning too koad Dec 23 '21 at 17:59
  • I will take your advice and do that. I put together a quick pastie of an idea that came to me at http://pastie.org/p/2uGFddOgNtKnV9OXLq3q94. It expires in 24 hours but I just wanted to get your quick thoughts on it before I put together a code review post. The idea here is to simply queue the calls and call them sequentially through the completion handlers of the async function. Is this a good starting point, in your opinion? You can plug that pastie right into a Playground. – lurning too koad Dec 23 '21 at 21:22
  • @kidcoder - That is not thread-safe. Setting that aside, if I wanted to implement it as a queue, rather than writing my own, I would use an operation queue, ideally suited for asynchronous tasks. E.g., I would wrap task in a subclass of `AsynchronousOperation` (shown [here](https://stackoverflow.com/a/48104095/1271826)), and then I could just add tasks to the operation queue with a `maxConcurrentOperationCount` of 1. That having been said, nowadays I wouldn’t use a queue at all, but rather use the async-await, define my database as an actor, and you're done. Much more elegant solution, IMHO. – Rob Dec 24 '21 at 16:59
  • I posted an attempt at an `Operation` subclass in Code Review per your suggestion. I'm not sure I have the KVO compliance syntax correct but if you can look at it that would be really cool. https://codereview.stackexchange.com/questions/272404/concurrent-operation-subclass-combining-kvo-compliance-and-thread-safety – lurning too koad Dec 28 '21 at 01:15