Add Sendable Conformances to Vapor #3000
Conversation
| private final class Cache: @unchecked Sendable { | ||
| private var storage: [ObjectIdentifier: Any] | ||
|
|
||
| private let storageLock: NIOLock |
There was a problem hiding this comment.
Use NIOLockedValueBox instead of storageLock and separate storage. The compiler can then infere the Sendable conformance and you can drop @unchecked. This will also allow you to make this a struct instead of a class as NIOLockedValueBox is already a reference type. Document that the struct has reference semantics now.
There was a problem hiding this comment.
I can't use NIOLockedValueBox though because of the use of Any right? I can't guarantee that anything being inserted into the storage is Sendable at which point NIOLockedValueBox is not Sendable
There was a problem hiding this comment.
True, but given that Cache is anyways @unchecked Sendable due to storage not being Sendable, it wouldn't matter if the NIOLockedValueBox isn't Sendable either, right?
There was a problem hiding this comment.
I see what you mean @ffried. It's not a huge deal either way.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3000 +/- ##
==========================================
- Coverage 76.72% 76.69% -0.03%
==========================================
Files 211 211
Lines 7781 8021 +240
==========================================
+ Hits 5970 6152 +182
- Misses 1811 1869 +58
|
Didn't expect to drop 5.7 before 5.6 LOL. A tiny (yet key) typo. |
There was a problem hiding this comment.
Came across this, waiting for it to drop :)
Left quite a few review comments (sorry for the noise!) - most of them are just small inconsistencies.
Maybe some of them aren't valid, e.g. if you always explicitly declare conformances (instead of relying on inheritance).
|
|
||
| public final class Locks { | ||
| // This does not have value semantics due to the use or `NIOLockedValueBox` | ||
| public struct Locks: Sendable { |
There was a problem hiding this comment.
Changing this to a struct is theoretically semver-breaking:
func useClass<C: AnyObject>(_ c: C) {}
useClass(Application().locks) // now failsCould be acceptable, though, since I don't see a reasonable use-case for such a function.
There was a problem hiding this comment.
This is very much a breaking change. You could do access let locks = application.locks and any access to locks would mismatch the state in application.locks
There was a problem hiding this comment.
@Joannis I don't think so. All properties of Locks have reference semantics (NIOLock is a class and NIOLockedValueBox has documented reference semantics). So Locks itself essentially still has reference semantics as well.
| private final class Cache: @unchecked Sendable { | ||
| private var storage: [ObjectIdentifier: Any] | ||
|
|
||
| private let storageLock: NIOLock |
There was a problem hiding this comment.
True, but given that Cache is anyways @unchecked Sendable due to storage not being Sendable, it wouldn't matter if the NIOLockedValueBox isn't Sendable either, right?
| @@ -12,19 +12,20 @@ extension Application { | |||
|
|
|||
| public struct Clients { | |||
| public struct Provider { | |||
There was a problem hiding this comment.
Shouldn't Provider become Sendable?
| @@ -22,7 +22,7 @@ extension Array where Element == CodingKey { | |||
| } | |||
|
|
|||
| /// A basic `CodingKey` implementation. | |||
| public enum BasicCodingKey: CodingKey, Hashable { | |||
| public enum BasicCodingKey: CodingKey, Hashable, Sendable { | |||
There was a problem hiding this comment.
CodingKey already requires Sendable.
| @@ -24,7 +24,7 @@ public struct File: Codable, Equatable { | |||
| } | |||
| } | |||
|
|
|||
| enum CodingKeys: String, CodingKey { | |||
| enum CodingKeys: String, CodingKey, Sendable { | |||
There was a problem hiding this comment.
CodingKey already requires Sendable.
| self.run = run | ||
| } | ||
| } | ||
|
|
||
| // This doesn't need a lock as it's only mutated during app configuration | ||
| final class Storage { |
There was a problem hiding this comment.
Can become Sendable as well (like other Storage's).
| @@ -22,7 +22,7 @@ final class ApplicationTests: XCTestCase { | |||
| } | |||
|
|
|||
| func testLifecycleHandler() throws { | |||
| final class Foo: LifecycleHandler { | |||
| final class Foo: LifecycleHandler, @unchecked Sendable { | |||
There was a problem hiding this comment.
@unchecked is not needed, is it?
| /// It can be used to capture local mutable values in a `@Sendable` closure and mutate them from within the closure. | ||
| /// As the name implies, the usage of this is unsafe because it disables the sendable checking of the compiler and does not add any synchronisation. | ||
| @usableFromInline | ||
| final class UnsafeMutableTransferBox<Wrapped> { |
There was a problem hiding this comment.
This is only used in this file as far as I've seen. Maybe make it fileprivate for now to prevent mis-use?
|
Will this PR fix the Vapor
|
|
@chibombo the idea is yes but this PR won't be merged. We've been adding Sendable stuff in piece-by-piece as we learn more about it etc, including the recent #3082 that just got merged. I think we're just down to Once that's in, the warning should disappear |
|
Closing this as we've been doing this in different parts |
Adds
Sendableconformances where appropriate. This removes almost all of the warnings when building Vapor with strict concurrency checking. Fixes a large number of concurrency and thread-safety issues caused by the introduction of Swift concurrency.Huge thanks to @dnadoba @Lukasa and everyone else who helped getting this over the line!
This also drops support for Swift 5.7 because of the use of
@preconcurrency