Skip to content
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 concurrency annotations to fix strict compilation warnings #155

Merged
merged 22 commits into from
Sep 13, 2023

Conversation

rebello95
Copy link
Collaborator

@rebello95 rebello95 commented Aug 22, 2023

This contains fixes for warnings that appear by enabling Swift strict concurrency in the Xcode 15 beta:

swiftSettings: [.unsafeFlags(["-Xfrontend", "-strict-concurrency=complete"])]

@rebello95 rebello95 marked this pull request as ready for review September 7, 2023 14:08
@rebello95
Copy link
Collaborator Author

@eseay this is ready for your review

@rebello95 rebello95 changed the title Fix Swift strict concurrency warnings Fix Swift strict concurrency warnings & add annotations Sep 12, 2023
@rebello95 rebello95 changed the title Fix Swift strict concurrency warnings & add annotations Add concurrency annotations to fix strict compilation warnings Sep 12, 2023
rebello95 added a commit to connectrpc/connectrpc.com that referenced this pull request Sep 12, 2023
Updates the docs to correspond to the changes being made for Swift concurrency in connectrpc/connect-swift#155.
Copy link
Contributor

@eseay eseay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rebello95 everything looks great! Honestly, despite the shear number of changes, the changes themselves are relatively straightforward.

One suggestion -

It may be helpful to clients implementing the library to provide a bit of guidance around how we are addressing making closures @Sendable in situations where you're wanting to update some kind of external state, but you need to do so in a thread-safe 'sendable' manner.

I know we want to keep Locked as internal as we can, but perhaps we can point to it as an example of how one can implement such a type themselves as well as an example of how we're using it in unit tests.

@rebello95
Copy link
Collaborator Author

Thanks for the review, @eseay.

It may be helpful to clients implementing the library to provide a bit of guidance around how we are addressing making closures @sendable in situations where you're wanting to update some kind of external state, but you need to do so in a thread-safe 'sendable' manner.

I know we want to keep Locked as internal as we can, but perhaps we can point to it as an example of how one can implement such a type themselves as well as an example of how we're using it in unit tests.

Hmm, I don't think this is a pattern that's unique to Connect though (I believe it's pretty common when using closures with Sendable) - where do you think this should live if we were to add it?

@eseay
Copy link
Contributor

eseay commented Sep 13, 2023

Thanks for the review, @eseay.

It may be helpful to clients implementing the library to provide a bit of guidance around how we are addressing making closures @sendable in situations where you're wanting to update some kind of external state, but you need to do so in a thread-safe 'sendable' manner.
I know we want to keep Locked as internal as we can, but perhaps we can point to it as an example of how one can implement such a type themselves as well as an example of how we're using it in unit tests.

Hmm, I don't think this is a pattern that's unique to Connect though (I believe it's pretty common when using closures with Sendable) - where do you think this should live if we were to add it?

You're definitely right in that it's not unique to Connect. I was more just thinking that it could be helpful for someone integrating Connect into an existing project where widespread familiarity with Sendable may not be the case. I personally had to do a little bit of digging to figure out the right solution for something like this when it came up in some of my other code.

Maybe we could add another example in the README that reflects one of the unit tests you've written where we're employing Locked, and we could just put some code comments around it that explain why the lock is necessary. And in that case, I'd say don't use Locked; use plain NSLock so as not to leak that further into the public API.

@rebello95
Copy link
Collaborator Author

Ah okay, that makes sense @eseay. I think that should probably live here, though: https://connectrpc.com/docs/swift/testing#using-generated-mocks

Is that okay with you? If so I can put a PR up for that. Would also appreciate another look at this PR - I pushed some changes per our offline conversation about allowing mocks to be open.

Copy link
Contributor

@eseay eseay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rebello95 LGTM!

@rebello95 rebello95 merged commit c041679 into main Sep 13, 2023
8 checks passed
@rebello95 rebello95 deleted the swift-concurrency branch September 13, 2023 20:33
rebello95 added a commit to connectrpc/connectrpc.com that referenced this pull request Sep 14, 2023
Updates the docs to correspond to the changes being made for Swift concurrency in connectrpc/connect-swift#155.
smaye81 pushed a commit to connectrpc/connectrpc.com that referenced this pull request Sep 18, 2023
Updates the docs to correspond to the changes being made for Swift concurrency in connectrpc/connect-swift#155.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants