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 via the forum messaging feature. When contacting the review manager directly, please keep the proposal link at the top of the message.
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?
Super important improvement for API authors that want to eventually align their runtime data-safety standards with Swift, but can't afford directly introducing crashes for existing end users.
if isIsolatingCurrentContext is not available on the expected executor, but checkIsolatedis available
What does it means by saying "be available", does it mean "the customized implementation of the method is used, not the default implementation", or, does it use some magical runtime introspection?
If I understand it correctly, it can be achieved by just using default implementations, for example:
It means "a non-default implementation" / "not the default implementation that exists in the stdlib" was provided by the developer. Yes, this is the compiler detecting this. Since we want to (soft-)deprecate the previous APIs and this one supersedes it we're taking these special affordances to be able to do this without breaking adopters.
This implementation approach sadly is not workable for the requirement we're facing: being able to issue warnings when able to.
If we'd just naively start calling isIsolatingCurrentContext with such default implementation when we'd want to issue a warning... We'd potentially end up calling checkIsolated, we'd cause a crash where we really wanted to only issue a warning if able to detect and issue a warning for such executor. This isn't acceptable for building tools or libraries which want to issue warnings and specifically not crash when unable to determine if a warning should be issued.
A specific use case here is frameworks which may have illegal usage patterns today and as they adopt Swift Concurrency want to start warning about those patterns which actually always were somewhat unsafe but "happened to work most of the time". They'd like to start issuing warnings for now, and maybe in future versions start crashing as developers had time to fix their code.
I want to dig into this more, because it came up in the pitch thread as well. The proposal makes use of the ability to detect whether isIsolatingCurrentContext was given the default implementation vs. the user providing that information, to decide whether it's safe to call checkIsolated(). However, it reserves that ability for itself: a user cannot tell whether isIsolatingCurrentContext provides a meaningful result, or whether it's getting the default false result.
I have three concerns about this aspect of the proposal:
It seems likely that users will want to build abstractions on top of isIsolatingCurrentContext that may need to be able to tell the difference. We shouldn't have to be tied in with the concurrency runtime to be able to implement something akin to "if isolated, do this; otherwise, do that" and be able to account for the "we can't know" case.
The proposed design requires specific, additional complexity in the compiler and runtime to handle this detection, when alternative solutions (such as the enum in the future directions discussion of this topic) model it with already-existing, well-understand language features.
While we were able to resolve the technical concerns that led to the checkIsolated() design previously with one library's executors, can we rule out that other libraries won't have this same issue and not find an appropriate solution? Is "I don't know" truly only a temporary thing that will go away, or might we need it in the future?
I know we want the Bool API, because it's simpler. But in doing so, we've created more complexity and shut out potential use cases in the process. This isn't a beginner API---indeed, most people shouldn't use it directly, but should use higher-level APIs like assumeIsolated instead---so I don't think we should shut out potential use cases to make it simpler. I'm much more concerned about being here again in a year or two generalizing the API a third time to accommodate those use cases than I am about each client of isIsolatingCurrentContext having to decide how to deal with the "unknown" case.
Primarily we ended up with the bool and dismiss the tri state because I didn't really see a meaningful way to handle "we don't know" as we'd generally have to assume it means "no" I think. Perhaps for some form of warning mode it could be useful to ONLY issue a warning if we concretely know that we're in the wrong isolation, but not if we don't know?
The only other implementation difference if we are to remove the special logic in the compiler and rely on the nil is that we will now always try to call the new method, and if "don't know" need to call checkIsolated next. Since this is the "failing path" it is likely okey to take the hit and make two (or three if there's complex executor equality) runtime calls. I think making a number of runtime calls is probably managable here but wanted to call this out that we'll be making more calls -- which can add up for things like as? with isolated conformances etc. But I think we're okey with those checks being more expensive.
In the future as most executors implement the new API we'd be back down to one check call; We only have to make two calls if we get the .unknown and then have to call checkIsolated as a fallback. That seems like an ok state to end up in the future -- most executors should be able to answer within a single runtime call.
--
Overall I think we can move to the tri-state, and the only thing to figure out then are the names of that. Currently the alternatives includes this type:
enum DetectedSerialExecutorIsolation {
case isolated // returned when isolated by this executor
case notIsolated // returned when definitely NOT isolated by this executor
case unknown // when the isIsolatingCurrentContext could not determine if the caller is isolated or not
}
I think we could change notIsolated to nonisolated as well, even though that's a runtime isolation and not the one typically associated with the nonisolated keyword, but it would make sense to rename that. isolated and unknown sound okey. And the type name is proably fine as well, it's not like this will be passed around much so a verbose name is okey here.
Good morning everyone,
it's been a while since this finished it's review period, however in the meantime we worked out some improvements I'd like to propose here.
The two changes I'm proposing here are:
changing the return type of isIsolatingCurrentContext to Bool?
this reduces special casing in the compiler as well as improves the usability of this API; nil signifies that an executor did not implement this check, and we cannot be sure if we are isolated to it or not.
adding a way to call this newly introduced API if all we have is an Actor
this comes in the form of Actor/withSerialExecutor which allows us to get to SerialExecutor/isIsolatingCurrentContext from e.g. an #isolation parameter or an @isolation(any) closure's .isolation.
These changes make this API more accessible from end-user Swift code, and not so tightly tied to the Swift runtime's internal uses.
Is there any reason not to make the generic return values of the two with closures T: ~Copyable? That would offer more flexibility to clients in case they want to return a non-copyable value.
I'm sorry for not seeing this before. I think we can simplify this API significantly to just be an Actor.executor property.
The goal of only exposing the executor to a callback appears to be to scope the access so that it cannot be used without also keeping the reference to the actor valid. I believe the proposal does this because the existing unownedSerialExecutor property is documented with the following odd requirement:
This property must always evaluate to the same executor for a given actor instance, and holding on to the actor must keep the executor alive.
This is clearly tying the lifetimes together in a way, and I can see how someone might come away believing that the executor cannot be safely used unless the actor object is kept alive.
However, that was not the intent, and in fact it is the opposite of the intent. The ownership half of this requirement is trying to say that returned executor is a borrowed result: that keeping the actor alive must implicitly keep the executor alive. This does not mean that executor cannot be separately managed as an object; it just means that the client does not have to separately manage the executor if it's already managing the actor. This is an important optimization which avoids retaining and releasing the executor every time it's retrieved. But if the client would prefer to manage the executor, it is free to do so, because the executor must still be an independently-manageable object. This is enforced in the Swift type system by the class constraint of Executor.
There's therefore no need for this API to be designed to specifically keep the actor object alive while the executor is in use; an ordinary getter is fine. At worst, we will have to independently retain and release the executor if we cannot be assured that the actor will stay alive.
Okay, so I hate to make this more complicated, but I have to ask the question... the most straightforward API here is:
var executor: any SerialExecutor
However... should we be making this an associated type of the actor protocol so that this can return a concrete type? It's annoying to do because we'll need to add a bunch of API surface area:
protocol Actor {
associatedtype ExecutorType: SerialExecutor
var executor: ExecutorType { get }
}
struct AnySerialExecutor: SerialExecutor {
let executor: any SerialExecutor
// ...
}
extension Actor where Self.Executor == AnySerialExecutor {
var executor: AnySerialExecutor {
AnySerialExecutor(executor: unsafeBitCast(unownedExecutor, to: SerialExecutor.self))
}
}
Well, for one, we can't really implement that for existing actor types. And for another, I'm concerned about that making actors commit to a specific executor implementation. (Perhaps we have this already to a limited extent for default actors, but I wouldn't want to double down on it.)
That doesn’t seem very safe to me, I’m not sure it’s worth it.
The way developers use an executor is by implementing the unownedExecutor. Not a typed one… so we’d have to do a whole dance about “you must implement the typed one” and derive the untyped one from it so it’s not wrongly retuning something else etc…
One “should not” do that but seems weird to make it easy to make such mistake and cause wrong casts hm
Existing actor types would get the default implementation that uses AnySerialExecutor as the associated type (I forgot to write out the default for the associated type).
They could always stick with AnySerialExecutor if they didn't want to commit.
I think I'd rather go with the simpler any SerialExecutor, still, and assume that anyone who wants to work with actors that have a specific concrete executor will define their own protocols that refine Actor and provide a concrete serial executor themselves.
Since so far we've not seen much need of expressing <A: Actor> where A.Executor: ... I don't think we need to jump onto that right now. It could be a future direction though if we ever see the need arise for real (maybe as executors get more "traits" or something).
After looking into this some more and discussing the safety aspects with @John_McCall I we think adjust the proposal to offer the following (instead of the with... method):
extension Actor {
var executor: any SerialExecutor { get }
}
extension DistributedActor {
var executor: any SerialExecutor { get }
}
which will return the unownedExecutor but as a SerialExecutor.
The conversion is actually not trivial -- it is not just a cast, and we'll have to do some work here, but it should be possible. So we don't need to introduce the withSerialExecutor function -- let's do a property instead.
The actual implementation of this is a bit tricky, but for the proposal's purpose I think we can assume we'll be able to implement it.
That's a bit deep in implementation details, but yes that is true specifically for default actors: "the executor identity" is actually the actor. You'd even be able to observe this today with unownedExecutor, where the identity is the actor pointer, though there is no witness table "ExecutorRef" when we do this, we just know it's a default actor then.
This is not the case when there's any custom executor in play -- the executor then is that object.
