SE-0340: Unavailable From Async Attribute

Evolution Proposal Reviews
1

Hi everyone. The review of SE-0340: Unavailable From Async Attribute begins now and runs through February 8, 2022.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager by email or DM. When messaging me directly, please keep the proposal link at the top of the message and put "SE-0340" somewhere in the subject line.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at https://github.com/apple/swift-evolution/blob/master/process.md.

Thanks for helping review this proposal!

Joe Groff
Review Manager

12 Likes
2

-1. I think this is an important feature, but it should be a separate attribute. Mixing it with @available feels like a hack.

When I see a declaration with an @available attribute, it tells me about when that declaration was introduced. The idea of broadening this to mean not only the availability of a particular library, but also the immediate context from which you want to call it, doesn't sit right with me.

I see from the proposal:

Some thoughts that prompted the move from @unavailableFromAsync to an availability kind include:

  • A given API may have different implementations on different platforms, and therefore may be implemented in a way that is safe for consumption in asynchronous contexts in some cases but not others.
  • An API may be currently implemented in a way that is unsafe for consumption in asynchronous contexts, but may be safe in the future.

I think it would be useful to provide examples of each of these, including the reverse case of APIs which were previously safe in async contexts but no longer are.

I also don't like the idea of requiring separately-named "safe from async" functions. It should be possible to say that a single function (which developers might already be using in non-async contexts) was not async-safe prior to some specific update. As I read the proposal, that is not possible with this design.

@available(macOS 12, noasync, *)
func someFamiliarAPI()

// Is this not a redeclaration of 'someFamiliarAPI'?
// All examples in the proposal use a different name for async-safe variants.
// That would be a really unfortunate pattern.

@available(macOS 12, *)
func someFamiliarAPI() {
  // async-safe implementation
}

I also don't think this design scales to other attributes which might want to include availability annotations. For example, consider the recently-pitched @noAllocations and @noLocks attributes - they do very similar things to this attribute, in that they prevent the function being called in certain contexts, and may want to attach availability clauses. Should they live in @available, too?

11 Likes
3

Also, given that the enforcement is deliberately weak, I think "unavailable" is the wrong term.

For example, if people are writing code which could actually be unsafe (as in: might invoke UB) in an async context, they might be tempted to reach for this, thinking that it will prevent such errors from happening. I mean, it's called "unavailable from async", and if they test it by trying to make a call from an async function, they'll see that it fails - so at first glance, it seems to check out.

But, since enforcement can be so trivially defeated, "unavailable from async" doesn't actually prevent that code from running in an async context. Again, this is very, very different from what people expect of @available - which is all about guarding people from ever using symbols that don't exist at runtime, and therefore must be undefeatable.

I would recommend something like @unadvisableFromAsync instead. To make it clear that nothing is really being prevented; at most, it's a hint to the caller that they might want to think twice.

1 Like
4

-1 Locks can be used safely in an async function so I don't think they should be marked unavailable from async. Ideally the compiler would be able to reason about them (even if it is just with pthread_mutex, os_unfair_lock and NSLock) and complain when actually locking across an await.

func doThis() async {
    pthread_mutex_lock(mutexPointer) // ok
    somethingSync()
    pthread_mutex_unlock(mutexPointer) // ok

    pthread_mutex_lock(mutexPointer) // ok
    await somethingAsync()
    pthread_mutex_unlock(mutexPointer) // error: locking across await
}
1 Like
5

+1 for the feature.

I would suggest using nonasync following https://github.com/apple/swift-evolution/blob/main/proposals/0097-negative-attributes.md

Edit: that proposal was rejected but we still ended up renaming noescape to nonescaping

1 Like
6

IMO the correct way to do such thing is to wrap up the API:

func doThis() async {
    withMutexLock(mutexPointer) {
        somethingSync()
    } // ok, withMutexLock(_:body:) accepts a sync closure

    withMutexLock(mutexPointer) {
        await somethingAsync()
    } // error
}
7

Blocking operations are not generally safe to use in async functions running on the default executor, because they will block a thread, and the cooperative pool for tasks has a limited number of threads for running tasks. You might be able to get away with using locks in limited circumstances, or when using a custom executor, but these cases are relatively limited, so having locking primitives be generally unavailable from async contexts and requiring a bit more work to allow in the limited circumstances they're OK seems like the right tradeoff.

3 Likes
8

Do all async functions necessarily run on the cooperative thread pool? And what about in the future when custom executors become a thing, then it may be perfectly ok to lock in async functions. It still seems to me the problem is not using locks in async functions but locking across awaits.

9

The vast majority of them do. As the proposal notes, though, you can get around the ban with a small amount of work in the cases you know it's safe to take a lock.

1 Like
10

I think this is conflating two things: async and the cooperative pool. But with
custom executors this is no longer true. Then I still don't think it is right to mark locks as unavailable from async.

11

I wonder, though, if that is the best way to do it. I don't know of any other language features which can be opted-out in this way.

For example, we have withoutActuallyEscaping, allowing you to opt-out of @escaping checking for closures which you know won't escape. This proposal seems to be the equivalent of:

// Oh no! This is escaping, but I want to use a non-escaping closure!
func takesEscapingClosure(_ x: @escaping () -> Void) { ... }

func takesNonEscapingClosure(_ x: () -> Void) {
  takesEscapingClosure(x)  // There. All good now?
}

If opting-out would involve creating a wrapper function anyway, why not make what it is doing explicit? For example, this is how you would actually write the escaping closure example today:

// Oh no! This is escaping, but I want to use a non-escaping closure!
func takesEscapingClosure(_ x: @escaping () -> Void) { ... }

func takesNonEscapingClosure(_ x: () -> Void) {
  withoutActuallyEscaping(x) { x in
    takesEscapingClosure(x) // âś… because withoutActuallyEscaping
  }
}

The performance annotations pitch uses a similar approach - withUncheckedPerformance (or something like that). You don't just wrap an allocating function in a function with @noAllocations and the compiler says that's fine.

3 Likes
12

The control-flow analysis is not trivial, though I may be able use a dominance query to ensure that every "opening" unavailable API is dominated by a "closing" API before an await or end node. I'll use "opening" that puts things into a bad state for usage across await boundaries or before the return of the async function, like pthread_mutex_lock. "Closing" would then put things back in a safe state, like pthread_mutex_unlock. We would need to encode the opening and closing API pairs (pthread_mutex_lock and pthread_mutex_unlock in this example) in the attribute or have some other way of indication what the pairing is, if there is one. I don't believe general semaphores are safe for consumption here. With C functions floating around, this may get a bit tricky.

The use of thread-local storage (TLS) is another issue in addition to locks. I suppose as long as we ensured that there is always a write to the TLS before a read after an await, it would be okay.
We would also need to annotate something like an open and closing API. What about in cases where we're calling out to a C function that operates on TLSs? Maybe a C function initializes a TLS, but only conditionally, so it's only sometimes an "opening" function. Do we need to fold that information into the attribute? The control-flow analysis questions here concern me.

Blocking issues would also need their own way to indicate things that shouldn't be thrown into the general cooperative pool, which was another point brought up in the initial pitch discussions.

The proposed approach can handle all of these cases.

As noted in the proposal, creating a wrapper around the API that you want to use is a totally viable way of providing the API in a way that is known to be safe. The example in the proposal looked something like:

func withPthreadMutexLock<R>(_ mutexPtr: UnsafeMutablePointer<pthread_mutex_t>, _ criticalSection : () throws -> R) rethrows -> R {
  switch pthread_mutex_lock(mutex) {
    case 0:
      defer { pthread_mutex_unlock(mutex) }
      return try criticalSection()
   case EINVAL:
      // ...
   case EDADLK:
      // ...
   case let value:
      // ...
}

Given the challenge and cost of the control-flow analysis and ensuring that it works correctly, the subtitles of the bugs introduced when the API are misused, and that wrapping API that should be wrapped, we settled on this as the design we wanted to go with.

13

@rokhinip on this thread says:

One other thing I'd like to highlight is that not all blocking is bad. If you are blocking on IO, or using an os_unfair_lock for data synchronization purposes in a tight critical section, the blocking here is temporary and therefore, fine. The effect is that you may have reduced throughput on the cooperative thread pool, but you will still be able to make forward progress.

Then why insist this much on marking locks as unavailable from async? I understand that using locks is not recommended, but having the compiler throw an error on them does not seem right when their usage is deemed "fine".

14

In the general case, it isn't okay. It's fine when paired correctly. The swift-y way to ensure this pairing is with a withFoobar( closure: () -> ()) like wrapper API.

15

And it is precisely for this reason that in the pitch and the proposal, we talk about replacement APIs that allow you to use locks safely in async code.

The current APIs are an easy foot-gun. The replacement API style that Evan talks about in his proposal makes this safer and allows you to use locks in async world just fine.

16

I get that but how do we expect programers facing an "unavailable from async contexts" error to understand what the correct alternative is and then get to it? Will the compiler diagnostic explain all this?

17

Yes. The attribute takes a renamed declaration name and an optional message. Both are presented as part of the diagnostic. The renamed declaration presents itself as a fix-it, replacing the bad call where it can figure it out. message just appends itself to the error message.

@available(*, noasync)
func doSomethingNefariousWithNoOtherOptions() { }

@available(*, noasync, message: "use our other shnazzy API instead!")
func doSomethingNefariousWithLocks() { }

func asyncFun() async {
  // Error: doSomethingNefariousWithNoOtherOptions is unavailable from
  //        asynchronous contexts
  doSomethingNefariousWithNoOtherOptions()

  // Error: doSomethingNefariousWithLocks is unavailable from asynchronous
  //        contexts; use our other shanzzy API instead!
  doSomethingNefariousWithLocks()
}
18

-1

I don’t think @available is an appropriate way to convey this information. Yes, a separate attribute would work very similarly, but there are many things with identical implementations that should nevertheless be separate.

Reusing the implementation behind the scenes in a compiler would be fine, but they should be distinct at the language level.

I’m not entirely convinced that this even needs a language feature. Given the difficulty of actually checking this, and the expected rarity of use, couldn’t this simply be handled by expecting problematic code to name itself “unsafe” and document the requirement?

I would also like to point out, as others already have, that this seems to overlap considerably with the @noLocks attribute from the performance roadmap, and that has the benefit of being more descriptive.

2 Likes
19

The current proposal makes it sound like locks will only get the bare error message:

@available(*, noasync)
func pthread_mutex_lock(_ lock: UnsafeMutablePointer<pthread_mutex_t>) {}

// Error! pthread_mutex_lock is unavailable from async contexts
pthread_mutex_lock(mutex)
20

What is your evaluation of the proposal?

Positive yet the proposal lacks ambition.

This is a "nudging" proposal, which aims at teaching users that some apis should not be used from async context, hoping:

  1. Users are lucky enough to call those apis right from the body of an async function, see the compiler error, and learn.
  2. Users remember to not use those apis in sync functions as well (but this time without compiler guidance)
  3. Users look for calls of those apis in their code base in order to spot misuses that compiler could not catch.
  4. Users infer from a compiler error on, say, pthread_mutex_lock that there exits other such apis, search for their list somewhere on the Internet, and apply steps 2 and 3 to those apis as well.
  5. Sometimes those apis are hidden in third-party code, and it is impossible to locate the wrong uses. Instead, the third-party has to consider that their public apis may be used from async contexts, apply step 4 on their whole code base, tag their public apis accordingly, and update their own documentation so that step 4 can be run on it.

That's a lot of hopes.

And step 4 implies the need for a place somewhere in the Swift documentation(s) for the list of those apis.

Do we favor Documentation or Luck?. This is the crux of the issue.

For example, I'm amazed by how lucky I am I could stumble on the list of apis that create priority inversion problems, after so many opportunities for reading "don't do that this creates priority inversion", without any further guidance. "Don't do that" is not helpful. The problem to solve is still there even after I'm told that I don't solve it well.

Is the problem being addressed significant enough to warrant a change to Swift?

IMHO, the problem exists, but requires documentation improvements, more than compiler guidance. Compiler guidance can surely help, though.

Does this proposal fit well with the feel and direction of Swift?

Weak guards are uncommon in the language.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I'm in favor for slight touches of nudging, but in this case, the proposal relies much too much on luck, and does not mention the documentation updates it requires for achieving any efficiency at solving the problem it claims to address. I'm afraid we'll "call it a day" after this proposal is implemented, when, in practice, the situation would not be much better at all.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Just enough experience with nudging apis to recognize one when I see it, and understand some consequences.

4 Likes