Make Request Sendable #3093
Make Request Sendable #3093
Conversation
|
This was surprisingly easy in the end. Tried to reduce the locks where possible, I've added individual locks to where it's likely to be accessed concurrently |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3093 +/- ##
=======================================
Coverage 76.23% 76.24%
=======================================
Files 211 211
Lines 7904 7998 +94
=======================================
+ Hits 6026 6098 +72
- Misses 1878 1900 +22
|
There was a problem hiding this comment.
Looks good but some requested changes.
I also see one probably-resolvable warning in "Development" on 5.9:
/__w/vapor/vapor/Sources/Development/routes.swift:274:36: warning: converting non-sendable function value to '@sendable (Request) async throws -> some AsyncResponseEncodable' may introduce data races
asyncRoutes.get("opaque", use: opaqueRouteTester)
| @@ -1,33 +1,94 @@ | |||
| import NIOHTTP1 | |||
| import RoutingKit | |||
| @preconcurrency import RoutingKit | |||
| import NIOConcurrencyHelpers | |||
|
|
|||
There was a problem hiding this comment.
I would hasten to point out that there's nothing held by Route that inherently makes it non-Sendable in theory- or for that matter, that justifies it being a reference type rather than a value type in the first place (or for that matter, that justifies it being mutable at all!). I know that can't be changed without breaking API, but it does suggest that @unchecked Sendable ("I know what I'm doing, punish me if I hork it up") might be a better choice than a bunch of locking no one should be hitting to begin with.
There was a problem hiding this comment.
The only thing that requires this here is PathComponent which we can make Sendable and I'll do in another PR when I tackle RoutingKit. That one I want to be very careful with though for obvious reasons
There was a problem hiding this comment.
Wrt to the whole thing of Route and unchecked Sendable I'm planning on doing a better audit of Vapor things, deprecating thing and then introducing a new AsyncRoute in it's place
|
@MahdiBM that's an annoying Swift bug with complete concurrency checking and nothing we can do at the moment until it's fixed in Swift |
|
O it's that bug... you can just open up the closure and the bug will go away. Though I'm fine with it either way since it's a bug. |
|
These changes are now available in 4.86.0 |
Final stage of Vapor's
Sendablejourney asRequestis nowSendable.There should be no more
Sendablewarnings in Vapor, even with complete concurrency checking turned on.