Add Sendable support - Githubissues

kean / Get

Web API client built using async/await

MIT License

937 stars 74 forks source link

Add Sendable support #19

Closed ericlewis closed 2 years ago

ericlewis commented 2 years ago

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.

wmorgue commented 2 years ago

Bump.

kean commented 2 years ago

Hi, does it require Swift 5.6?

ericlewis commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

Info about preconcurrency attribute: https://github.com/apple/swift-evolution/blob/main/proposals/0337-support-incremental-migration-to-concurrency-checking.md
Sendable is a little harder to find info about but it essentially boils down to "safe to transfer across concurrency domains". The link above might be enough to explain what is going on.

kean commented 2 years ago

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 commented 2 years ago

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.

kean commented 2 years ago

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

ericlewis commented 2 years ago

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.