[Pitch] Make noasync annotation transitive

Evolution Pitches
1

Currently you can mark functions with something similar to:

@available(*, noasync, message: "This can block the calling thread and should not be called in an async context", renamed: "asyncAlternative()")

This will produce warnings by the compiler for anytime you call a function with this annotation in an async context. However if you call this in a new function and then call that function in an async function, you get no warning. E.g.:

@available(*, noasync, message: "This can block the calling thread and should not be called in an async context", renamed: "asyncAlternative()")
func noAsyncFunc() {
  // Do something that shouldn't be done in an async context
}

func regularFunction() {
  noAsyncFunc()
}

func doSomethingAsync() async {
  noAsyncFunc() // This is bad and produces a warning (or error in Swift 6)
  regularFunction() // This is bad but produces no warning
}

You need to manually audit your code and find all of the possible noasync calls, which is not a simple task if you have dependencies that are annotated as well. This is very error prone and seems to be a bit of a gap in the safety of Swift. I propose that noasync functions should produce transitive warnings so all these are automatically caught by the compiler.

This has come from our work in Vapor. We call a number of functions in NIO that are now marked with noasync due to the changes required by Swift Concurrency. The APIs we expose that use these functions are not marked with noasync and can cause issues. The only way for us to ensure we're doing the right thing is check every single function call to make sure it's not calling a noasync function, or wait for someone to discover a crash or issue.

14 Likes
2

Big +1 from me, this seems like a genuinely useful addition.

To give an example of the expected outcome, should this function be paired with an appropriate warning?

func regularFunction() { // Warning: `noAsyncFunc` is marked as noasync, but `regularFunction()` is not. This can allow it to be called in async contexts, which is an error in Swift 6.
  noAsyncFunc()
}

Or should the error bubble to the final call site? If we bubble it, do we expect the warning to cross module boundaries?

I also wonder where the failure modes of this assumption lie — ie. is there a world were a function can do shenanigans to allow noAsyncFunc() to still be called safely (at the expense of a manual audit), but also silence this new warning?

4 Likes
3

This was discussed in the proposal review.

I even tried (unsuccessfully) to advocate for a model where noasync would be transitive up to an explicit marker when you said it was async-safe again, to address the case of async-unsafe functions composing to something that is safe.

3 Likes
4

Maybe someone from the language team can chime in and see if these assumptions still hold a couple of years later.

We're seeing that mixing existing APIs that are wrapping noasync APIs to silence the warning makes it incredibly easy to write unsafe code. E.g. as soon as we start trying to adopt NIO's custom executors, a Vapor app will crash and the only way for us to ensure this isn't the case is a manual audit which is error prone and very un-Swift like.

IMO the warning combined with Swift 5 language mode (ie it will still compile in Swift 6 until you're ready) is not too disruptive.

6 Likes
5

I think bubbling the error up gets complicated if the function calls multiple noasync functions. So it should probably be something generic like "enclosingFunction contains calls to functions that are marked as unavailable from async contexts"

6

This would probably need something like the equivalent of @unchecked Sendable so maybe:


@available(*, async)

To tell the compiler you're handling this. But this gets complicated because whilst you might be able to handle problems that arise in the called function (like ensuring no async hops) you can't anticipate issues arising from the callee (specifically thinking of the error from NIOs custom executor)

3 Likes
7

Following up on this - one easy thing that is currently catching people out is that defer swallows any noasync warnings, because it's essentially a synchronous closure, which is causing runtime crashes as users aren't getting alerted to the noasync warnings

4 Likes
8

To me that case sounds like a concurrency bug that we ought to fix in Swift 6.

6 Likes
9

I can add an issue to GH with a reproducer if that helps! What would the desired outcome be? A warning on the noasync function call inside the defer?

1 Like
10

Yeah I'd say the behavior should match whatever you'd get without the defer by 'manually' duplicating the code just before every scope exit.

11

I don't thik we can just re-purpose the existing noasync to become viral because that's not right. For example, Lock.lock() and Lock.unlock() functions are usually marked noasync but their (correct) composition func withLock<R>(_ body: () throws -> R) rethrows { self.lock(); defer { self.unlock() }; return try body() } should not be noasync (as it automatically prevents the illegal lock, then await, then unlock).

This however does not mean that we don't need a viral version of noasync also. Some functions can block indefinitely and compositions of indefinitely blocking functions shouldn't be used in async contexts.

So I'd rephrame this as a feature request for a viral version of noasync on top of the existing non-viral noasync. (Apple folk, this is rdar://106241745.)

3 Likes
12

Right, but isn't that that the general pattern that "safe things can be composed of unsafe things" (and in fact often are, when you get to the lowest levels)?

I think noasync being "viral" by default is not just useful but the only sensible option; it feels like a flat-out bug that it's not.

But also, there should be a way to tell the compiler that a function is async-safe even if it uses noasync subfunctions or types. Convention would seemingly dictate a withUnsafeNoasync function, or similar.

4 Likes
13
14

We keep discovering crashes caused by the lack of this. What would the next steps be for pushing this?

15

This brings up an interesting case that we probably need to account for, one I've just hit in Vapor.

Say you have a function that looks like:

func doSomethingAsync() async {
    // This is fine as you say 
    self.application.locks.lock(for: Key.self).withLock {
        if let existing = self.application.storage[Key.self] {
            return existing
        }
        let new = HTTPClient(
            eventLoopGroupProvider: .shared(self.application.eventLoopGroup),
            configuration: self.configuration,
            backgroundActivityLogger: self.application.logger
        )
        self.application.storage.set(Key.self, to: new) {
            // This is not - this will cause a crash - should the compiler be aware that this is an issue and is this even possible to detect if we don't make all `noasync`s viral?
            try $0.syncShutdown()
        }
        return new
    }
}
16

Big +1 from us. We have a ton of old GCD code that, while it is well documented, can easily be called without a forethought, especially with lower knowledge base on modern async/await Swift concurrency best practices.

I'm about two steps from modifying thousands of functions to propagate these, a transitive attribute would be HUGE for us. transitivenoasync. Someone else used the term viral lol I like that too.

1 Like