Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for function call order expection #297

Open
Kyle-Ye opened this issue Mar 16, 2024 · 9 comments
Open

Add support for function call order expection #297

Kyle-Ye opened this issue Mar 16, 2024 · 9 comments
Labels
concurrency Swift concurrency/sendability issues enhancement New feature or request public-api Affects public API

Comments

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Mar 16, 2024

Description

For async call or blocking call, we want to test the executing order. I'd like to propose an API set similar to confirmation

Expected behavior

public struct Order4 {
   public let 1: Child
   public let 2: Child
   public let 3: Child
   public let 4: Child

   public struct Child {
      ...
   }
}
extension Order.Child {
  public func callAsFunction() {
     ...
  }
}

func order2<R>(_ body: (Order2) async throw -> R)
func order3<R>(_ body: (Order3) async throw -> R)
func order4<R>(_ body: (Order4) async throw -> R)

@Test
func orderTest() {
  order3 { order in
    order.1()
    await Task {
      ...
      lock.wait()
      order.3()
    }
    await Task {
      ...
      order.2()
      lock.broadcast()
    }
  }
}

Actual behavior

No response

Steps to reproduce

No response

swift-testing version/commit hash

No response

Swift & OS version (output of swift --version && uname -a)

No response

@Kyle-Ye Kyle-Ye added the enhancement New feature or request label Mar 16, 2024
@grynspan
Copy link
Contributor

Most scenarios that might need something like this are also solved by Swift concurrency, which is why we didn't implement an interface for it. For instance, if you expect functions A, B, and C to execute in that order, async/await means it's usually possible to force that to happen just by writing your calls to them in the right order.

Is there a scenario you're thinking of where that isn't possible today?

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 17, 2024

Most scenarios that might need something like this are also solved by Swift concurrency, which is why we didn't implement an interface for it. For instance, if you expect functions A, B, and C to execute in that order, async/await means it's usually possible to force that to happen just by writing your calls to them in the right order.

Is there a scenario you're thinking of where that isn't possible today?

Yes. For normal async/await Swift concurrency scenarios, there is no problem.

But for C-Style like POSIX lock API, it does not support very well.

The Task and await code are just pseudo code to create 2 pthread. The core issue of the above example is to test the lock's behavior where Swift concurrency is not available.

However, such scenario should be very rare. Almost only a few low-level libraries that need to deal directly with the C language need such usage.

If upstream does not plan to support this feature, I totally understand the concern.

@grynspan
Copy link
Contributor

swift-testing is, of course, a Swift testing library. C code that behaves well when imported into Swift is testable too, but esoteric C behaviour is beyond the library's purview. That's not a no to this feature request though: do you have a real-world example of C code that you need to use from Swift which you cannot turn into async/await code using continuations, tasks, task groups, etc.?

Something we could look at doing is using variadic generics and adding an overload of confirmation() à la:

await confirmation("first thing happened", "second thing happened", ..., strictlyOrdered: true) { first, second, ... in
}

This would help with the "arrow of death" pattern too.

@grynspan grynspan added concurrency Swift concurrency/sendability issues public-api Affects public API labels Mar 18, 2024
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 19, 2024

swift-testing is, of course, a Swift testing library. C code that behaves well when imported into Swift is testable too, but esoteric C behaviour is beyond the library's purview. That's not a no to this feature request though: do you have a real-world example of C code that you need to use from Swift which you cannot turn into async/await code using continuations, tasks, task groups, etc.?

The example is the lock API usage in almost every large iOS projects (NSLock, os_unfair_lock and so on).

We have a wide usage of them in our ObjectiveC and Swift codebase. And we probably can convert them into Swift Concurrency but it's not on the plan.

From https://developer.apple.com/videos/play/wwdc2021/10254/?time=1507
Primitives like os_unfair_locks and NSLocks are also safe but caution is required when using them. Using a lock in synchronous code is safe when used for data synchronization around a tight, well-known critical section.

The core issue is we'd like to write test case for them easily. The current test implementation is a little tricky and unstable.

Something we could look at doing is using variadic generics and adding an overload of confirmation() à la:

+1 for this idea/implementation. Look forward to seeing the landing PR.

@grynspan
Copy link
Contributor

So, for NSLock and os_unfair_lock/OSAllocatedUnfairLock, the primitive locking/unlocking functions are barred from use in async contexts and you should use the withLock {} API instead. This API has a built-in guarantee that the order of operations is correct, which eliminates the need for you to write a test that checks the order of operations. Hooray! 🥳

I can certainly understand that there is code out there that cannot use the provided withLock {} API (swift-testing itself can't for layering/dependency reasons) but since these APIs must not be used in an async context anyway, simply ordering your calls correctly is sufficient to ensure they execute in the right order.

That's not me saying "no, we won't build this feature", to be clear! But if we have a concrete, real-world use case that isn't trivially solved without the API, then that makes this more urgent to implement (vs. being a nice addition to the API, but low-priority.)

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 19, 2024

So, for NSLock and os_unfair_lock/OSAllocatedUnfairLock, the primitive locking/unlocking functions are barred from use in async contexts and you should use the withLock {} API instead. This API has a built-in guarantee that the order of operations is correct, which eliminates the need for you to write a test that checks the order of operations. Hooray! 🥳

Sorry for the misunderstanding caused here. What we actually want to test here is something lower level than withLock.

The example is our cross-platform lock/condition implementation and we want to test the wait and broadcast API here.

https://developer.apple.com/documentation/foundation/nscondition/1407950-wait
https://developer.apple.com/documentation/foundation/nscondition/1415094-broadcast

// Thread 1
func thread_1() {
    ...
    lock.wait()
    order.3()
    ...
}

// Thread 2
func thread_2() {
    ...
    lock.wait()
    order.4()
    ...
}

// Thread 3
func thread_3() {
    ...
    order.2()
    lock.broadcast()
}

@Test
func orderTest() {
    ...
    order.1()
    // Kicks thread 1 and thread 2's work
	...
	// Kicks thread 3
    ...
}

After broadercast is called we'd like to confirm the wait is over and make it continue to process its task.

@grynspan
Copy link
Contributor

I would not recommend using NSCondition in Swift, especially in concurrent code. I know that's not a very satisfactory answer, but NSCondition is generally incompatible with Swift concurrency because it may cause a thread owned by Swift's concurrent thread pool to block indefinitely or even deadlock.

It is generally safe to assume that NSCondition itself already has unit tests in Foundation that check that it behaves as designed, so you don't need to write your own unit tests just to check NSCondition itself.

Consider reimplementing your code to use Swift concurrency instead of a condition lock, and you may find that you (again!) don't need a dedicated interface in swift-testing to ensure your code operates correctly. 🙃

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 19, 2024

It is generally safe to assume that NSCondition itself already has unit tests in Foundation that check that it behaves as designed, so you don't need to write your own unit tests just to check NSCondition itself.

Got it. But the problem is we are using our own implementation of XXCondition instead of Apple's Foundation.NSCondition. So I think we'd better add test case for it.

I would not recommend using NSCondition in Swift, especially in concurrent code.

Got it. But that's a fairly old code in the codebase we cannot have a rewrite. I guess the best solution is just do not make the new Swift code to have a dependency on it.

Anyway thanks for your time on this issue. If upstream has plans to support such API later, we can keep this issue open, otherwise let's just close it.

@grynspan
Copy link
Contributor

We can keep this issue open as there's still value in the proposed API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Swift concurrency/sendability issues enhancement New feature or request public-api Affects public API
Projects
None yet
Development

No branches or pull requests

2 participants