Semaphore cancellation should increment semaphore value

[KaiOelfke]

I found a few things:

1. I think the semaphore should be incremented before throwing in the case of an early cancellation.
2. The current tests don't cover incrementing the semaphore, when cancelling. When commenting out the value += 1 line in main or lines with the changes from this PR for incrementing the value the tests still pass.
3. Many tests fail when running on an iOS simulator or with LIBDISPATCH_COOPERATIVE_POOL_STRICT=1 enabled. This is because of wait(for:timeout:) blocking the cooperative thread pool.

---

1. The fix is simple:

if value >= 0 {
    defer { unlock() }
    // All code paths check for cancellation
    if Task.isCancelled {
        value += 1
        throw CancellationError()
    }
    return
}

---

2. For this I added tests. I also added some helper code to avoid the problem of 3.

---

3. I added an async helper here for timeouts in my test. It's also possible to fix tests by awaiting tasks before waiting for certain expectations.

func test_wait_suspends_on_zero_semaphore_until_signal() async {
  // ...
  let task = Task {
      await sem.wait()
      ex1.fulfill()
      ex2.fulfill()
  }
  
  // Then the task is initially suspended.
  wait(for: [ex1], timeout: 0.5)
  
  // When a signal occurs, then the suspended task is resumed.
  sem.signal()
  await task.value
  wait(for: [ex2], timeout: 0.5)
}

But after experimenting with this I think it's not nice to use wait in async test functions, if test destinations with a cooperative thread pool size of 1 should be supported. Ideally there would be some async version of wait from XCTest that doesn't block. But as far as I know this doesn't exist.

[groue]

Hello @KaiOelfke,

Thanks for the pull request: you found a bug indeed 👍

The pull request contains a lot more than a fix, though.

In particular, I don't understand the many added let task = ... await task.value changes. What do they change?

How does the completeWithin(nanoseconds:) helper enhance the built-in XCTest expectations?

What is the purpose of LIBDISPATCH_COOPERATIVE_POOL_STRICT?

I appreciate that you feel at ease with the project, because not many people do so, and not many people try to improve it :-) I'm just trying to limit the changes to the useful ones.

[KaiOelfke]

When I started experimenting with the code by default Xcode was running the test scheme on an iOS simulator. The size of the cooperative thread pool seems to be one in this scenario on my machine, in debug and with Xcode 14.1 and because of this some unit tests fail.

LIBDISPATCH_COOPERATIVE_POOL_STRICT is an environment variable explained in this WWDC talk:

https://developer.apple.com/videos/play/wwdc2021/10254/?time=1611

If you enable this you can run tests also on a macOS destination with a pool size of 1. Without this normally the thread pool size would be bigger. Then the same tests fail.

So this is the problem I described under point 3.

Why does this problem happen?

It's explained in the WWDC talk as forward progress. If you have a cooperative thread pool size of 1 and the one thread can't make forward progress all async code stops to run.

wait(for:timeout) is blocking, if the expectations aren't yet fulfilled. This means it breaks the forward progress requirement.

So how can this be fixed?

Either don't use the wait API and something different like the helper function I added. Or guarantee that expectations are always fulfilled before calling wait(for:timeout). The latter is why I added the let task = ... await task.value. This make sure the task, which fulfills an expectation is finished before wait.

I recommend you to run unit tests on main with LIBDISPATCH_COOPERATIVE_POOL_STRICT or on an iOS simulator to better understand this PR.

[KaiOelfke]

How does the completeWithin(nanoseconds:) helper enhance the built-in XCTest expectations?

It's async / non-blocking (just suspending) so it doesn't break the forward progress rule.

[groue]

Thank you very much for the learning material, this is very kind of you 🥰 Please stay tuned until I let this sink in. Automated testing on the various platforms and the variable sizes of their thread pools is definitely something this repo should be able to achieve :)

[dannys42]

+1 for this bug & fix. my preferred variation is this:

         value -= 1
        if value >= 0 {
            defer { unlock() }
            // All code paths check for cancellation
            do {
                try Task.checkCancellation()
            } catch {
                value += 1
                throw error
            }
            return
        }

to ensure we don't mask the error thrown by checkCancellation.

[groue]

OMG @dannys42, you are my soulmate 😉 edab432

Of course, thank you @KaiOelfke for spotting the issue, and apologies to everyone for my very slow reaction.

I won't merge the PR. I agree there are improvements to bring, but I'd don't want this lib to become a showcase of smartness, or that anyone feels 🤯 when reading the code. So, thank you again, but I'd prefer a more humble style.

The fix has shipped on the main branch, version 0.0.7.

[KaiOelfke]

@groue

No worries I also prefer easier to read code.

The issue about failing unit tests on devices with a thread pool of size 1 still remains though. I think It's important to support this size as e.g. Apple Watch or iPhone simulators have such a single thread thread pool currently.

Just use LIBDISPATCH_COOPERATIVE_POOL_STRICT=1 as environment variable or run tests on a Simulator to reproduce this.

A custom helper like my completeWithin or wrapping things in await Task {} is no longer necessary from Xcode 14.3 as Apple shipped fulfillment(of)

See also the 14.3 release notes on this issue:

Fixed: XCTestCase.wait(for:timeout:enforceOrder:) and related methods are now marked unavailable in concurrent Swift functions because they can cause a test to deadlock. Instead, you can use the new concurrency-safe XCTestCase.fulfillment(of:timeout:enforceOrder:) method. (91453026)

Note that this is an issue only with the test code and not the library code.

[groue]

Yes, thank you for this kind reminder. That's really where I have to catch up and learn from your techniques.