Strict Concurrency (alternative) #302
Conversation
You bet! I'll try and take a look within the next week or so. A cursory glance tells me I think I like this direction better than where I was headed. However, I want to get it inside Xcode to see what's up and maybe even try it with the latest Swift 6 dev snapshot for kicks. |
|
@rafael-assis @bachand could you run this version on your test suit to see if the performance is affected? And also to evaluate if the new syntax causes any warnings / requires tests migration |
| ViewHosting.expel(function: function) | ||
| expectation.fulfill() | ||
| Task { @MainActor in | ||
| ViewHosting.expel(function: function) |
There was a problem hiding this comment.
I'm thinking the inspection should happen in the task as well, so as to align with previous behavior.
| private static var sanitizedNamespacesCache: [String: String] = [:] | ||
| private final class Cache: @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| var replacedGenericParameters: [String: [String: String]] = [:] |
There was a problem hiding this comment.
IMO use private variables and make the internal accesses to this forcibly protected by the lock.
Future maintainers may not realize that reading/writing to these cache values isn't thread safe. As written this type isn't actually Sendable, so it could cause issues.
Example of what I mean:
private var _replacedGenericParameters: [String: [String: String]] = [:]
var replacedGenericParameters: [String: [String: String]] {
get { lock.protect { _replacedGenericParameters } }
set { lock.protect { _replacedGenericParameters = newValue } }
}There was a problem hiding this comment.
Yeah, this is a valid concern. On the other hand, locking just inside getter and setter does not eliminate the issue completely, as the code that reads, modifies, and then writes back the value has to run under a single critical section (lock). If not, a concurrent flow can modify the value right after we read, so a subsequent write would not set the correct value already.
| @@ -110,9 +117,8 @@ public struct Inspector { | |||
| ] | |||
|
|
|||
| private static let sanitizeNamespaceRegex = { | |||
| // swiftlint:disable force_try | |||
| // swiftlint:disable:next force_try | |||
There was a problem hiding this comment.
total NIT, if you inline this it's easier for IDE functions (like raising/lowering the line)
try! NSRegularExpression(pattern: sanitizeNamespacePatterns.joined(separator: "|")) // swiftlint:disable:this force_try
| @@ -11,7 +11,28 @@ public extension Locale { | |||
| } | |||
| ```` | |||
| */ | |||
| static var testsDefault: Locale = Locale(identifier: "en") | |||
| nonisolated | |||
There was a problem hiding this comment.
GitHub might be having a hard time tracing this through, but I don't think this type is an Actor, right? So I don't think the nonisolated keyword is necessary.
| if Thread.isMainThread { | ||
| return try assumeIsolated(action) | ||
| } else { | ||
| return try DispatchQueue.main.sync { |
There was a problem hiding this comment.
Using sync on the main queue is usually a great way to cause a deadlock. IMO we'd be better off just using assumeIsolated without the check and else branch. assumeIsolated should already give good diagnostics if it's not run on the main thread to help somebody trace through. This could hide those diagnostics behind weird deadlocks -- especially in XCTest which tends to make some interesting choices about what's run on the main thread.
There was a problem hiding this comment.
Good point, I'll change it
Hey @nalexn, sorry for not responding sooner. Yes, we will try this next week and report back. Thank you for thinking of us! |
nalexn
added
the
pending release
Hi @nalexn, I apologize for the long delay in responding here. I wanted to report that @bachand and I have run all of our test suites against the latest commit of the repo (e8b5fac) and everything went smoothly. We didn't observe any performance degradation or warnings. 💯 We appreciate you letting us know about the change that could impact our environment that consumes Thank you so much! 🙏 |
nalexn
removed
the
pending release
This is a follow-up PR for #291 that possibly replaces #298 and addresses the concerns from this comment.
@MainActorattributes in the public API, making it better compatible with the existing pre-concurrency test suits@Tyler-Keith-Thompson I cannot assign the review to you, but I hope you'll get a chance to look at this. Everyone else is welcome to review as well