Skip to content

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

NeedleInAJayStack
Copy link
Member

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).

paulofaria
paulofaria previously approved these changes Jun 24, 2025
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).
@NeedleInAJayStack NeedleInAJayStack changed the title Draft: feat!: Uses swift concurrency under the hood BREAKING: Converts to Swift Concurrency Jun 25, 2025
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.
This was causing test hangs on macOS
@NeedleInAJayStack
Copy link
Member Author

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?

@NeedleInAJayStack NeedleInAJayStack force-pushed the feat/swift-concurrency branch from db77ae5 to c398797 Compare July 5, 2025 06:15
@NeedleInAJayStack NeedleInAJayStack force-pushed the feat/swift-concurrency branch from c398797 to 34268d3 Compare July 5, 2025 06:17
paulofaria
paulofaria previously approved these changes Jul 14, 2025
@NeedleInAJayStack
Copy link
Member Author

@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 NeedleInAJayStack changed the title BREAKING: Converts to Swift Concurrency Breaking: Converts to Swift Concurrency Jul 21, 2025
@paulofaria
Copy link
Member

@NeedleInAJayStack one last question about enabling strict concurrency checking. Have you tried that?

@NeedleInAJayStack
Copy link
Member Author

@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!

@NeedleInAJayStack
Copy link
Member Author

NeedleInAJayStack commented Aug 4, 2025

@NeedleInAJayStack one last question about enabling strict concurrency checking. Have you tried that?

@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 unchecked a bit more than I would like. In particular, allowing circular references in the GraphQLSchema type system requires allowing the client to mutate the fields of a GraphQLObjectType, which inherintly makes it non-sendable. Reworking it as an actor is possible, but gives us an async explosion. The cleanest solution is probably to have a "definition" step where types are mutable and not sendable, and an "build" step that converts the entire schema to immutable types. I'm leaning toward just leaving it unchecked and documenting that the schema should not be mutated after starting to execute against it, because 1) it keeps us close to the JS reference implementation and 2) most people are using it this way already (and Graphiti's patterns strongly promote this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants