Enable strict concurrency checking in CI #132
Conversation
|
@swift-server-bot add to allowlist |
| @@ -27,13 +27,17 @@ final class ActorTracingTests: XCTestCase { | |||
| func work() async {} | |||
|
|
|||
| actor Foo { | |||
| var bar = 0 | |||
| let bar: LockedValueBox<Int> = .init(0) | |||
There was a problem hiding this comment.
This part is why adding @sendable to the operation closure is wrong, the withSpan must not be sendable and thus the bar does not have to do synchronization -- this would make tracing a complete pain in actors otherwise.
I.e. this patch by adding Sendable here forces this locking or causes errors warning: actor-isolated property 'bar' can not be mutated from a Sendable closure; this is an error in Swift 6 which we must not be causing here.
There was a problem hiding this comment.
Right, when I remove sendable from the closures in withSpan, I get:
Passing argument of non-sendable type '(any Span) async -> ()' outside of actor-isolated context may introduce data races
and
Passing argument of non-sendable type '() -> ServiceContext' outside of actor-isolated context may introduce data races
One for each closure being passed in. How should we fix that?
There was a problem hiding this comment.
Actually, is the answer here to duplicate the withSpan methods in extension Actor, which implement the methods again using the building blocks of startAnySpan etc? That seems to make the warnings go away for me.
There was a problem hiding this comment.
But why would you want the operation to be sendable? We're always going to invoke it right away and never store it anywhere for "execute somewhere else"; they don't need to be sendable
There was a problem hiding this comment.
I'm not saying they should be sendable :) I'm trying to fix the warnings I pasted above, and now exploring the idea of the extension on Actor.
There was a problem hiding this comment.
I'm a bit surprised the extension methods + Sendable does not cause warnings? Either way I think the answer here is just that operation closure just isn't sendable and that's ok 🆗 😄
There was a problem hiding this comment.
The problem is that this test will then produce warnings when complete concurrency checking is enabled. And client code that tries to do this as well. I think we need some solution here (even if making the closures sendable isn't the right one), the code, as written, produces warnings with strict concurrency checking, today.
There was a problem hiding this comment.
We're backed into a corner here to be honest. Complete checking is strict, but the language does not offer right tools to solve this. The workaround (that even the stdlib uses) is done via @_unsafeInheritExecutor but that's incredibly unsafe and we cannot use it here -- we'd literarily be introducing real dataraces in correct looking code (this attribute removes ALL hop-to operations).
I'd be too worried to put it onto withSpan which is intended to wrap "entire functions" because it'll end up causing actual race conditions at runtime -- by now making any actor hops.
I think we must wait it out and get to the safe version that would become @inheritExecutor and only then would complete checking be correct here... (and in the stdlib as well for that matter).
As potential workaround, I have explored the "overload" on Actor but it also hits a number of problems. We could do this: 9484f4b
I talked this over with @FranzBusch and @Lukasa a bunch, and end up coming back to the safe @inheritExecutor whenever we hit those issues. This should probably inform the priority of finding a solution for these issues as we plan for enabling complete checking in the near future.
There was a problem hiding this comment.
Understood - I'll back out the public API change and we should just land this PR with this warning, and address it separately. I wouldn't want to introduce new public API unless we want to maintain it long term.
There was a problem hiding this comment.
Overall good, but we must not change the operation to be sendable, it breaks how tracing is intended to be used, comment here: https://github.com/apple/swift-distributed-tracing/pull/132/files#r1293083353
Once we revert changing operation closure types this should be okey. I'm also unsure why context closure would want to be sendable, seems iffy 🤔
|
Thanks for the review, @ktoso, will address your feedback! |
|
@ktoso, ok I commented out the strict concurrency flags for now, but I think taking these changes would still be valuable. Once we have an answer for making |
|
It's a good change over all, I'll give it another skim and merge. Related radars: rdar://113846321 rdar://113848512 |
| } | ||
|
|
||
| /// Returns the globally configured ``Instrument``. | ||
| /// | ||
| /// Defaults to a no-op ``Instrument`` if ``bootstrap(_:)`` wasn't called before. | ||
| public static var instrument: Instrument { | ||
| self.lock.withReaderLock { self._instrument } | ||
| shared.instrument | ||
| } | ||
| } | ||
|
|
||
| @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal ServiceContext | ||
| extension InstrumentationSystem { | ||
| /// INTERNAL API: Do Not Use | ||
| public static func _findInstrument(where predicate: (Instrument) -> Bool) -> Instrument? { |
There was a problem hiding this comment.
TODO: We can SPI this, I'll take that on :)
|
Looks good, thank you @czechboy0 ! And we'll follow up on those radars about strict checking and closures |
Summary
To help avoid concurrency bugs, this PR enables complete concurrency checking mode and addresses all the current concurrency warnings.
Some notable changes:
stdout/stderr(which emitted a warning, as it's a read-write global variable), usingfdopen, which I think should be fine, but calling it out./scripts/validate_format.sh, so might be worth reviewing the diff with?w=1Test Plan
Tested that all builds and tests pass on macOS and Linux.