-
-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Sendable conformances to WebsocketKit #131
Conversation
@Lukasa if you want to give this a once over to make sure I'm not doing anything stupid that would be great! |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
- Coverage 85.78% 83.93% -1.86%
==========================================
Files 6 6
Lines 626 641 +15
==========================================
+ Hits 537 538 +1
- Misses 89 103 +14
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits. Also fix that horrible lifecycle test in Vapor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM at this point, with some caveats:
- With all the
@Sendable
additions, I wonder if we should require Swift 5.7. Reasoning: NIO wraps such additions in#if swift(>=5.7)
, despite@preconcurrency
having been added in 5.6; I don't know why this is. - Definitely wouldn't mind a quick eyeballing of this from @Lukasa.
- Need to deal with that deadlocking test.
Ok @gwynne I think we're finally good and I've fixed the race conditions in the tests |
Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>
These changes are now available in 2.10.0 |
* Add dev containers to gitignore * Revert "Revert "Add Sendable conformances to WebsocketKit (#131)" (#135)" This reverts commit 0b43ce5. * Add @preconcurrency annotations to work around unsafe users * More preconcurrency annotations * Add API breakage allowlist * Add missing preconcurrency annotations * Last missing annotations * Update allowlist --------- Co-authored-by: Gwynne Raskind <gwynne@vapor.codes>
Bump minimum Swift version to 5.6 and add Sendable conformances where appropriate