I don't think this deserves bringing in a new dependency. A Bool behind a Lock should be fine.

Surprisingly the Swift Atomics README does not explain why you would want to use that package instead of just using mutexes. The reason is that using OS locks is several orders of magnitude slower than the atomic operations provided by Swift Atomics. That is due to this:

all atomic operations map directly to dedicated CPU instructions where available -- to the extent supported by llvm & Clang.

This post from the ArangoDB site shows the high cost of OS locks vs. atomic operations: Comparison: Lockless programming with atomics in C++ 11 vs. mutex and RW-locks

For this issue there is no need for optimal performance. But I was expecting atomics to be used to solve some of the various data races being reported by the ThreadSanitizer. Some of those involve operations that occur frequently. I've not dug into those issues yet. It may be that those could be solved by consistently queuing to a particular thread, rather than sharing between threads.

I can pull the use of Swift Atomics and change AtomicBoolean to use a lock. If we find a case were simple typed data is frequently accessed between two threads we could then decide if we should add use of Swift Atomics.

Does that sound good to you?

I worked as an iOS performance engineer in the past, so I'm familiar with atomics and how they are used :) In this case I just don't think it is worth bringing in a dependency. When Swift adds these natively we can switch over to them, and until then we can always drop down to C if necessary to access these.

Understood. Right now I'm thinking with the event driven approach and main thread dispatching access to the new flags can be restricted to the main thread, eliminating the need for this class in this case.

If it proves to be needed, I'll propose a version that uses the new Lock class and not Swift Atomics.

It would be good to get PR #3938 merged (logger improvements) as that one adds the Lock class.

Blocking a thread on a semaphore is typically not very polite on Darwin. Can we try rethinking this API and having work be queued instead?

I too have been questioning my own decision to implement it this way. I don't think queuing would work. The correct way to avoid this is to toss the current thread based code in AppDelegate.applicationShouldTerminate in the trash and implement an event driven state machine to control shutdown. This is the offending code:

    terminateQueue.async {
      // When a window is closed it stops playback. That involves sending a stop command to mpv.
      // That command executes asynchronously. MUST wait for that command to complete. If the quit
      // command is sent to mpv while the stop command is still executing the commands can
      // interfere and cause the playback position to not be saved in the watch later file. See
      // issue #3939 for details.
      for pc in PlayerCore.playerCores {
        pc.waitForStopToFinish(timeout: timeout)
      }
      // Now that playback has been stopped terminate the players.
      for pc in PlayerCore.playerCores {
        pc.terminateMPV()
      }
      // Player termination involves sending a quit command to mpv. That command executes
      // asynchronously. Wait for all the players to finish terminating before proceeding.
      for pc in PlayerCore.playerCores {
        pc.waitForTermination(timeout: timeout)
      }
      // Tell Cocoa to proceed with termination. This has to be done on the main thread.
      // The main dispatch queue MUST NOT be used to avoid a deadlock when quitting is
      // initiated by mpv in response to the user pressing "q". When that happens
      // MPVController submits a task to the main queue that calls terminate. That call
      // blocks the main dispatch queue.
      if #available(macOS 10.12, *) {
        RunLoop.main.perform(inModes: [.common, .eventTracking, .modalPanel]) {
          self.proceedWithTermination()
        }
      } else {
        RunLoop.main.perform(#selector(self.proceedWithTermination), target: self,
                             argument: nil, order: Int.min, modes: [.common])
      }
    }

The advantage of this thread style solution is that it is very clear what the shutdown process is doing as the sequence is right there in the code. An event driven solution is definitely better, but frequently not immediately obvious as to how the code works.

For an event driven system I envision something like the following...

PlayerCore would post notifications when the stop and quit commands finish. Early on AppDelegate.applicationShouldTerminate would add observers for those actions. When an event indicating stop as completed the handler in AppDelegate would have PlayerCore send a quit command. When an event indicating quit has completed the in AppDelegate would poll all the cores to see if they all have finished terminating and if so then proceed with termination. Something like that.

Unless you have second thoughts I'm thinking I should go ahead and change the implementation to use such an event driven approach.

I take back my comments about waiting on a semaphore, the application is going to quit anyways and I don't really think it matters that this doesn't perform all that well. That said, I don't really like exposing this primitive as API, as we did with Lock (which I consider to be generally useful and desirable). Can we set up an ad-hoc dispatch group here and use it to control the forward progress of this thread?

Originally I was thinking an event driven solution would require more changes and be more harder to understand. After thinking more on how that could be done I'm convinced event driven is the proper approach. That plus smarter use of dispatching to the main thread should be less complex than the current approach. The event driven approach eliminates the need for AtomicBoolSemaphore. By dispatching to the main thread I should be able to eliminate the need for AtomicBool.

Busy today, but should be able to post such a change later this week.

Re: AppDelegate.applicationShouldTerminate():
If you haven't yet invested time in refactoring it again, consider this dispatch group example. You might want to create a DispatchQueue with attributes: .concurrent for it, because DispatchQueues are serial by default. It shouldn't be too hard to implement, and you can also call wait() with a timeout.

But you must guarantee that exactly one enter() is called for every leave() - an error will be thrown when leave() is called if { numEnters - numLeaves } is negative. (I have taken to using defer {} blocks around the leave() in case there's any possibility of exceptions occurring).

So you can probably do something like this:

// do the same for stop above
// ...
let terminateQueue = DispatchQueue(label: "terminate", attributes: .concurrent)
let terminateGroup = DispatchGroup()

for pc in PlayerCore.playerCores {
  terminateGroup.enter()
  pc.terminateMPV()
  terminateQueue.async {
    defer { terminateGroup.leave() }
    pc.waitForTermination()
  }
}
terminateGroup.wait(timeout: .now() + 10)

Note that in this design, you'll still need the semaphores so that you can wait on them.

A little worried about savePlaybackPosition() getting called here (or at all - I thought the mpv core was already handling the save?) But there are some holes here:

• It's a good start that you are setting isMpvTerminated = true right after guard !isMpvTerminated else { return }. But what you really want is both of those in one atomic operation (e.g., getAndSet()), because having them apart leaves open the possibility of two threads operating at nearly exactly the same time, where ThreadA does the guard (false), then ThreadB does the guard (false), then both threads set the bool to false and continue executing the rest of the method in duplicate. This works in some languages but is generally considered an anti-pattern: see ("The Double-Checked Locking is Broken" Declaration)
• You've got two independent flags, isStopped and isMpvTerminated, but you're mixing the logic between them. Although terminateMPV() first checks isMpvTerminated and then savePlaybackPosition() checks isStopped before saving the file, the latter flag doesn't get set until playbackStopped() is called. So a call to terminateMPV() which is quickly followed by a call to stop() can result in two calls to mpv.command(.writeWatchLaterConfig) in quick succession.
• Also keep in mind that isStopped can change back from true to false, if fileStarted() is called. So you must consider the possibility that this flag may change values at any time and in any direction, which further complicates things.
• And unlike isMpvTerminated, there is a long delay between when the stop is started and isStopped becomes true, which increases the chances for race conditions.

Rambling suggestions:

1. Try to decouple the stop logic shutdown logic. If savePlaybackPosition() must be called, make it private, and only called by stop(). Have terminateMPV() call stop() before it does its own work - where stop() either does nothing (if isStopped is true) or does a regular stop (if false).
2. The call to stop() should do nothing if either isStopped or isMpvTerminated is true.
3. The call to terminateMPV() should be a composite operation. It should (a) do nothing if isMpvTerminated is true, or (b) if it is false, call stop(), which either (c) does stop if isStopped is true, or (d) does nothing if it is false.
4. The method mpvHasShutdown() should set isStopped - it is outside its domain.
5. Ideally try to just put a lock (or DQ task block) which encompasses both the "get" for a flag in the beginning and the "set" for (possibly) each at the end. Generally, any time you go between locked blocks, you need to check all the state flags all over again.

Actually, I realized you actually have 2 more tasks, because you need to wait for both stop and termination... if you're doing a state machine, it would be a lot easier... Now this is getting really rambly.

Example state flow which includes a stop:
NOT_STARTED -> fileStarted() -> STARTED -> stop() -> WAITING_FOR_STOP -> playbackStopped() -> STOPPED -> fileStarted() -> STARTED

Example state flow which includes a termination:
NOT_STARTED -> fileStarted() -> STARTED -> terminateMPV() {isTerminating = true; stop()}-> WAITING_FOR_STOP -> playbackStopped() { state = (isTerminating ? TERMINATION_REQUESTED : STOPPED) } -> TERMINATION_REQUESTED -> mpvHasShutdown() -> TERMINATED

A call from an invalid state (for example, if fileStarted() was called when already in the STARTED state, I suppose the easiest thing for now would be to spit out an error message but otherwise ignore it. There seems to be a danger of massive scope creep.

But what you really want is both of those in one atomic operation (e.g., getAndSet()

YES! You are pointing out one of the reasons why I mentioned I'm worried the "fix" is not deterministic. To make this fully deterministic requires locking similar to the proposed logger fix in PR #3938. I started down that path, but was concerned about all the changes required to do that. In particular, how do you handle "get" operations that occur after mpv has been told to shutdown? If designing from scratch probably you would make all results optional and return nothing. A lot of work to retrofit that.

A different approach can be seen in the replacement PR. That one takes the attitude that this method must only be called by the main thread. So the thread in MPVController processing mpv events has to queue calls to PlayerCore to the main dispatch queue. Probably some internal checks should be added to ensure use of the main thread.

I too am used to making a class "iron clad". Written from scratch a call to stop after quit would be an internal error. Always painful to try and add such enforcement after the fact. In this case AppDelegate is supposed to ensure that does not happen. I am concerned about the flow if a file is being opened right as IINA starts to shutdown.

Initially I was thinking an event driven state machine and ended up with the thread based termination sequence seen in this PR. The new PR is event driven, but does not have a state machine.

At the moment there is no coordination with a high chance a race condition causes a crash. I'm trying to fix that enough so failures are unlikely without all the changes required to fully refactor the code. Basically trying to avoid the massive increase in the scope of changes needed to really nail this down.

For a fully "safe" system we'd have to figure out what to do about mpv deciding on its own to shutdown, as in when the user sends a quit command directly to mpv. That opens a window where mpv is shutting down and IINA doesn't know that until mpv sends the shutdown event. IINA might access mpv in that window triggering the same kind of crash we are trying to fix.

Re: AppDelegate.applicationShouldTerminate():
If you haven't yet invested time in refactoring it again, consider this dispatch group example. You might want to create a DispatchQueue with attributes: .concurrent for it, because DispatchQueues are serial by default. It shouldn't be too hard to implement, and you can also call wait() with a timeout.

But you must guarantee that exactly one enter() is called for every leave() - an error will be thrown when leave() is called if { numEnters - numLeaves } is negative. (I have taken to using defer {} blocks around the leave() in case there's any possibility of exceptions occurring).

So you can probably do something like this:

// do the same for stop above
// ...
let terminateQueue = DispatchQueue(label: "terminate", attributes: .concurrent)
let terminateGroup = DispatchGroup()

for pc in PlayerCore.playerCores {
  terminateGroup.enter()
  pc.terminateMPV()
  terminateQueue.async {
    defer { terminateGroup.leave() }
    pc.waitForTermination()
  }
}
terminateGroup.wait(timeout: .now() + 10)

Note that in this design, you'll still need the semaphores so that you can wait on them.