-
Notifications
You must be signed in to change notification settings - Fork 73
Breaking: Converts to Swift Concurrency #166
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
base: main
Are you sure you want to change the base?
Breaking: Converts to Swift Concurrency #166
Conversation
b6afc7d
to
cd2009d
Compare
This removes the NIO dependency. It is breaking because it removes all Swift NIO-isms that were present in the public APIs (like EventLoopFuture and EventLoopGroup argument/return types).
cd2009d
to
cf9f195
Compare
cf9f195
to
ac8c049
Compare
ac8c049
to
9ec313a
Compare
The intent is to replace it with swift-distributed-tracing integration.
This resolves the race condition caused by the inbox counts and the event delivery. If event delivery happens before the subsequent publish increments the inbox counts, then the counts will be lower than expected. Resolved by just not asking for inbox counts, since they aren't relevant to the test.
6a3bb81
to
1bc642c
Compare
This was causing test hangs on macOS
Hey @adam-fowler & @paulofaria - I managed to get the tests passing, and I think this is ready for you guys to look again. Could you give it a pass on my changes following your comments? |
db77ae5
to
c398797
Compare
c398797
to
34268d3
Compare
4094273
to
e1c66d9
Compare
@paulofaria Okay, I'm feeling pretty confident in this - do you mind taking a look at the final changes and approving (again)? Thanks for the review!! |
@NeedleInAJayStack one last question about enabling strict concurrency checking. Have you tried that? |
Good point - I've poked at it a bit but haven't really figured out the size of the effort. Supporting strict concurrency will be a breaking change, so probably best to do it now if it's feasible. I'll take a look this weekend and see how much work it is. Thanks Paulo! |
@paulofaria I got almost everything working with strict concurrency checking in this commit (compiling successfully, just 4 tests failing). That said, I am throwing around |
This removes the NIO dependency. It is breaking because it removes all Swift NIO-isms that were present in the public APIs (like EventLoopFuture and EventLoopGroup argument/return types).