In my code base I am using a global actor across my code base and using swift testing to tests various parts. Swift tests is configured to run tasks in parallel.
I noticed some of my tests are flakey and failing randomly. From my investigation I suspected this could be because of global actor instance being shared across tests impacting execution order. This led me to making global actor task local:
@globalActor
public actor MyGlobalActor: GlobalActor {
@TaskLocal
public static var shared = MyGlobalActor()
}
and injecting unique instance with a TestScoping trait. With this changes I see these tests passing without any error (yet to see any failure). Is this kind of pattern supported, e.g. overriding global actor instance with task local scope, or am I just seeing false positives in my tests?
Does your global actor have mutable state? Could it be that you're sharing that mutable state between tests?
If not, I do think having an actor per test isn't a bad idea since it means that one test doesn't have to wait on another as all the tests try to serialise on the one actor.
I did not realise you could override global actor isolation via TaskLocal. That's kind of wild, but I suppose it makes sense.
Yes I have mutable state that is isolated to global actor but I am not sharing any mutable state. All my functional tests use a single scope that provides unique state with various TaskLocal values.
The tests that are failing are mostly async tests and also doesn't fail if I use .serailized tag from swift testing. Hence I was suspecting the failure could be due to the re-entract nature of Swift actors.
I did not know this either, just had a wild idea and tested this. Seems to be working for my use-case.
I get your concern but I am not overriding the actor in my production code. The only override happens in a swift testing scope and the same scope overrides any shared state that are global actor isolated. IMO it should be safe (I am still running repeated tests and verifying this), would love to hear some thoughts as well how this could go wrong.
I do not believe it is possible to guarantee safety even if these tests are run serialized. A single task could still create child tasks internally that rely on a global actor being a singleton. That might not happen, but the language does need this to be an invariant.
I promise I don't want to make your life more difficult, but unfortunately this must be a compiler bug and should fail to compile. I think you are almost certainly correct that you have reentrancy issues here.
(I personally believe we must go even further and completely disallow global actor instantiation altogether, but that is a different discussion.)
I think it would be helpful to allow such kind of overrides only in test targets, or when module built with testable flag enabled. For other cases I think compiler can give errors for such usages.
I don't think this is an emergency or anything, but I do think it would be both possible and reasonable for the compiler to better enforce this contract. At a minimum, because these represent data race safety holes.
there's various ways to break the contract, even by computed properties.
Would it be possible to plug the holes if we said that GlobalActor conformance checking for the shared property only accepts conformers that define it as a static let ? This way, it can't be mutated or be a computed property. With -swift-version 6 , it being a let will prevent the accessor macro @TaskLocal from being applied to it, too.
I'm not sure about ever making it an error, but the warning on not-let might be reasonable since we now have @diagnose with which we can ignore it if certain that it's fine...
A bit tangential, but what convinced you that this particular pattern is worthy of a diagnostic? IIUC there are many ways you can introduce data-race safety holes by failing to uphold semantic requirements that the runtime expects (and documents). E.g. the same argument here could be applied to implementations of unownedExecutor too, right? Not necessarily suggesting it shouldn't be done, but curious where you feel the line should be drawn and what caused you to change your mind in this instance.
I don't think it can. The compiler has made a promise of statically-enforcing data race safety. I interpret this to mean, as an example, that if it is possible for the compiler to determine at build time that a global actor is not, in fact, a global, it also has assumed the responsibility to produce a diagnostic. There are cases, of course, where an error may be source-breaking and judgement needs to be used.
The new continuation type is a further example pushing the system towards more safety at the type-system level.
But I do think this is very distinct from violating a runtime invariant. Because while the compiler has also taken considerable steps to validate data-race safety at runtime too (as dynamic actor isolation checks do), there are technical limits to what is possible here.
There's many places where you can do "bad things"™ especially with executors, but executors we think are a more expert audience that should watch of what they're doing. Executors basically never check if you hold them right in any way.
This pattern was tricky because it looks all correct, it is just using plain old actors, but you suddenly broke the contract and concurrency safety. I feel like people reaching for "just" actors should be still on the paved path and be given more help than people implementing executors who absolutely must read the fine print.
It's an arbitrary distinction, but in this case doing the diagnostic after chatting with @aviva some more was the right balance. Unless someone disagrees during reviews
Sure, but in this case, the proposed diagnostic logic does not really try very hard to determine if the implementation semantically upholds the contract – it will just flag any non-let implementation. We can come up with simple (though contrived) examples that are semantically correct but will be flagged, e.g.
@globalActor
enum TaggedMainActor<T> {
static var shared: MainActor { MainActor.shared }
}
// Finding use cases left as an exercise to the reader...
To be clear, a complicated analysis to cut down on spurious diagnostics here does not seem at all worth it (and is in full generality probably impossible), so the tradeoff seems reasonable. But, since it's not an error, it won't definitively rule out the problem.
I'm not sure why you think unownedExecutor implementations aren't basically the same with respect to this category of problem (breaking documented semantic contracts that aren't enforced). This compiles cleanly, but is completely broken semantically:
import Foundation
@globalActor
actor A {
static let shared = A()
nonisolated var unownedExecutor: UnownedSerialExecutor {
DispatchSerialQueue(label: "q").asUnownedSerialExecutor()
}
}
But I may be misunderstanding... perhaps your position is that all these other cases should also be diagnosed in some manner.
That seems like it may be a reasonable place to draw the line, at least given how things currently are. I appreciate your elaborating on your thought process. Also, the extra docs accompanying the proposed diagnostic group to explain this situation seem like they'd be a nice addition.
My 2c is that emitting a warning is desirable here, since (like the start of this thread) you could mess this up by accident. The compiler can guess (correctly in many cases) that you didn't mean to do this. I think almost all global actor impls should be the static let shared = form?
Tangent: its not what we use @unsafe for today in Swift afaik, but in Rust an implementation that must be correct to uphold safety is done with unsafe impl which I think is a quality worth copying. It isn't obvious (to me, without reading docs) that implementing global actor wrong would break race safety, so its good for the compiler to try to let you know about this with a warning, as a stopgap.
Absolutely and there are probably more. I think it is fine to decide that deeper analysis here is a low priority.
There might be some existing, correct global actor implementations that do not have this form. But I'd be interested to look more closely at them, because I still hold the position that there should be stricter compile-time rules here.
(And as a bonus, I think adding those rules could enable other desirable behavior).
How should the compiler reason about the semantic problem here? Does it have an existing means of modelling the behavior of UnownedSerialExecutor and DispatchSerialQueue? I think both of those things would be necessary, at an absolute minimum, to detect this problem.
One reason someone might want a computed shared is it could be useful to track the “provenance” of the @MyActor call. I briefly considered using this in my library node-swift (to control the way the executor behaves based on a task local, in fact).
But even if done in a thread-safe manner, I wasn’t able to convince myself that the language makes any guarantees as to how/when it will invoke shared, therefore making a computed shared pretty useless afaict (IIUC it would be perfectly legal for Swift to cache it the first time one accessed it).
Given the fact that 1) it’s seemingly not very useful and 2) it’s more often than not a footgun (appears to enable UB even under StrictMemorySafety), I’d like to offer my +1 to a diagnostic on static var (and hey, at least a sufficiently determined user has the ability to suppress those now.)
