Using `async` functions from synchronous functions (and breaking all the rules)

To be clear, this is the only way for Vapor to achieve their stated goal of keeping their old synchronous APIs around while basing them on the new asynchronous ones. While that's unfortunate, it's not possible to design this away, because it's inherent in the problem. A synchronous operation must block the calling thread until the operation is complete, and an asynchronous operation may delay execution.

The only ways out of this mess are:

  1. Remove the synchronous API
  2. Provide two separate implementations
  3. Drop actors and use locks directly, at the risk of a much less safe implementation

But there's nothing the Swift concurrency team can do to make this easier: the complexity is inherent in the problem domain.

4 Likes

I looked at the vapor pull request and it seems actors are mostly used here to gate access to simple dictionaries, that are then used in many places. I wonder:

  • is it worth the cost of going async for doing something as trivial as accessing a dictionary?
  • the actors are used in many places and cause a cascade of async contamination (the infamous coloring problem), this might contribute to the other infamous problem of interleaved code execution, is the use of such fine-grained actors really the right design?
  • let's be honest, it's not like using locks to gate access to dictionaries would be a difficult thing to do and produce much more unsafe code. I could even argue that avoiding the interleaved code execution problem produces safer code.

I understand the appeal for using actors, but is it really worth the cost in this particular case?

1 Like

If this is indeed merely for protecting array / dictionaries, using mutex lock or an equivalent is more than adequate and probably the fastest possible solution. It would be very hard (albeit possible *) to get into a deadlock situation in code like this:

func value(forKey: Key) -> Value? {
    mutex.lock()
    let value = dictionary[key]
    mutex.unlock()
    return value
}

func setValue(value: Value, forKey: Key) {
    mutex.lock()
    dictionary[key] = value
    mutex.unlock()
}

(*) unless there's some exotic scenario, like a custom EQ/hash key implementation that might in turn access a data structure that needs locking.

OTOH, rarely atomic protection of individual data structures is enough, more often several data structures need to be protected at once. Plus in real cases it would be more than just the above. Consider sort / filter / find as an example, where you need to supply a sorting / filtering / searching closure - and there's nothing exotic in that - the closure that might in turn touch other data structures that need protection. One "hammer" approach (though not optimal) to this - sort / filter, etc the copies, and change the original data structure atomically (like in the above get / set).

To clear up a few comments that have come up:

  • We're not discounting any implementation - our stated goals of safety and Sendable adoption in a non-breaking way are the goals. If there is a better way to do it other than the current approach then we'll take it
  • This is a very real possibility - leave the existing internals as they are (with probably the addition of locks for thread safety) and heavily nudge users to use the new APIs.
  • The current PR only has fairly simple stuff. However it's only going to get more complex as more internals get reworked. I don't want to get in the situation where we stick with something like looks and a) ship a deadlock or unsafe code that would have been caught by a compiler or b) box ourselves in for any required future changes.

We want to make use of Swift features, especially those that ensure both Vapor and Vapor's users write safer code with more errors pushed from runtime to compile time

This seems the safest choice. Also, it doesn't seem like more work since you are leaving your current API as-is and you have to implement the new one anyway.

I'm sad to see that the Swift concurrency story has us cornered into fear of using anything other than actors. Regardless, many awaits have been added to the code in places that just didn't have them before. Code that used to execute in one go may now suspend and execute re-entrantly, now I'm genuinely curious, did you audit the code and were able to confirm that this is a non-issue and the code is safe?

Have you tried the design I suggested upthread?

It seems to me that it is essentially what @ktoso is describing - running the operations themselves on a non-pooled thread, and having Swift concurrency callers enqueue their work with a continuation so you never block resources from the global pool.

Of course, you'll have your own prioritisation for the things you want to deliver as part of this change. Personally, thread safety would be my top priority, followed by exposing non-blocking async calls, followed by deadlock prevention as a "nice to have". Non-thread-safe code being accessed from multiple threads means undefined behaviour, and fixing that would be independently useful even if Vapor didn't use any other Swift concurrency features.

Those non-thread-safe resources will need some kind of mutual exclusion to serialise accesses (that's inherent to the problem, as has been mentioned), and IMO the most natural fit for a system which should eventually be based on language-integrated actors would be to use a dispatch queue. There are clear parallels between the private queue and the actor's internal "message box". Dispatch queues are also the primary way pre-Swift-concurrency async code is implemented today on Apple's platforms and elsewhere, so it's a familiar pattern to many Swift developers (which can be useful as these features roll out and perhaps bugs emerge which the community will want to help fix).

I believe that actors are a good basic design tool and the right way to structure the high-level concurrency in many systems — if you're exposing a type with a concurrent interface, I think you should be strongly considering making that type an actor. But there are definitely still a lot of uses for synchronous locks, especially in the implementation of systems that intend to largely provide synchronous interfaces. More importantly, Swift will eventually have some sort of native lock type; we've focused on actors for good reasons, but I don't want people to avoid locks just because actors feel more native right now. If you have a well-designed lock type (like Apple's OSAllocatedUnfairLock, although I personally would like that a lot more if it really committed and forced you to use the withLock API that provides the state inout), you should feel free to use it fearlessly. Locks are less composable tools than actors in some ways, and you generally need to be more thoughtful about what work you do while holding them, but the guarantee of synchronous behavior is a very powerful tool that's sometimes just what you need.

(And there's a more general point there; good concurrency design includes understanding when to deliberately make things synchronous, e.g. in order to force them to happen uninterrupted, or to force clients to "come prepared".)

I'm not trying to pass judgment on the APIs in question here; I haven't looked at them at all. If there's a good reason for them to become async, then absolutely, make them async. I just wanted to clarify how I, personally, think about how actors fit into a design and when to use sync/async functions.

13 Likes