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 Sendable support #19

Closed
wants to merge 2 commits into from
Closed

Add Sendable support #19

wants to merge 2 commits into from

Conversation

ericlewis
Copy link
Sponsor

@ericlewis ericlewis commented Mar 1, 2022

  • Conform internal models to Sendable.
  • There are two classes which are unchecked Sendables and use an NSLock to manage access to the mutable values, which seems to be the recommended way according to the Swift forums.

Tests pass locally. Of course, the alternative is that we could just @preconcurrency import Get, but.. going to need proper support anyway and the locking mechanisms fix potential data races that exist today.

Conform internal models to Sendable. There are two classes which are unchecked Sendables and use an NSLock to manage access to the mutable values.
@wmorgue
Copy link

wmorgue commented Mar 21, 2022

Bump.

@kean
Copy link
Owner

kean commented Mar 23, 2022

Hi, does it require Swift 5.6?

@ericlewis
Copy link
Sponsor Author

ericlewis commented Mar 24, 2022

@kean I believe 5.6 is required for the @preconcurrency attribute, I can adjust the PR to work with previous versions of swift by doing something like this:

#if compiler(>=5.6)
@preconcurrency import Foundation
#else
import Foundation
#endif

Edit: went ahead and made the change

@kean
Copy link
Owner

kean commented Mar 24, 2022

I would appreciate if you could do that. In the meantime, I'll learn about Sendable and preconcurrency because I haven't worked with it yet.

@ericlewis
Copy link
Sponsor Author

ericlewis commented Mar 24, 2022

I would appreciate if you could do that. In the meantime, I'll learn about Sendable and preconcurrency because I haven't worked with it yet.

Went ahead and did it already

@kean
Copy link
Owner

kean commented Apr 3, 2022

Making request, response, and configuration Sendable seem like a good idea.

Unfortunately, it doesn't compile in Xcode 13.2 without @preconcurrency. I'm not sure how it can be implemented without it.

@ericlewis
Copy link
Sponsor Author

I would only need to change the compiler version I think, I don't happen to have a copy of 13.2 on hand however. So maybe the compiler version needs to be set to 5.5 instead of 5.6.


public struct Request<Response> {
public struct Request<Response>: Sendable {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be extension Response: Sendable where T: Sendable {}. We should restrict the users to only using Sendable types are responses.

@kean
Copy link
Owner

kean commented Apr 27, 2022

@ericlewis, I made Request and Response conditionally Sendable in 0.8.0 – commit.

I'm not sure if anything else needs to be. I don't think Configuration or APIDelegate need to be. And APIClient already is because it's an actor.

Thanks for opening an MR and raising this topic. I'm going to close this PR. If you have any suggestions, I'm open to them.

@kean kean closed this Apr 27, 2022
@ericlewis
Copy link
Sponsor Author

Sorry for only seeing this now - the reason that came up is because you can pass around api stuff through sendable boundaries. it should be somewhat safe to do so. but we get complaints. so making it safer was a good option.

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.

None yet

3 participants