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

Conform all models to Sendable protocol? #26

Open
ostaptyv opened this issue Aug 14, 2023 · 6 comments
Open

Conform all models to Sendable protocol? #26

ostaptyv opened this issue Aug 14, 2023 · 6 comments
Labels
wontfix This will not be worked on

Comments

@ostaptyv
Copy link

Currently, I'm working on my pet project, and I've turned on SWIFT_STRICT_CONCURRENCY = complete for my project. And here's what the compiler gives me:

image

I think it would be good to conform the models to the Sendable protocol in the context of the wide adoption of the actor-based concurrency model

@p2-apple p2-apple added the enhancement New feature or request label Aug 14, 2023
@p2-apple
Copy link
Collaborator

Great idea!

@p2-apple
Copy link
Collaborator

Ok, just had time to think about this. The resources themselves can't be Sendable because they are classes with mutable properties and inheritance. To change this – especially the inheritance – would be a large undertaking and breaking previous versions.

So: do you need the resource types to be Sendable for a specific reason? I don't see the code in your getPatients() method. but could that be a non isolated method and it internally uses isolated methods to access mutable state?

@ostaptyv
Copy link
Author

ostaptyv commented Aug 22, 2023

Sorry for the long reply, had a lot going on on my plate 😅

Nope, here's the full pseudocode of the actor:

actor PatientR4StorageImpl: PatientR4Storage {
    func getPatients() throws -> [ModelsR4.Patient] {
        let url = URL(string: "url/to/file")!
        let data = try Data(contentsOf: url)
        let bundle = try JSONDecoder().decode(ModelsR4.Bundle.self, from: data)
        let patients = bundle.entry?.compactMap { entry in
            entry.resource?.get(if: ModelsR4.Patient.self)
        }
        return patients!
    }
}

So answering your question: the method is isolated, and I still get this warning. As I mentioned earlier, the reason for this is because I've enabled SWIFT_STRICT_CONCURRENCY = complete in my Build Settings.

Currently, I don't see any problems with the fact that the models don't conform to the Sendable — it just would be nice to have. But I assume that this will almost certainly change as my project progresses

Maybe you could use @unchecked Sendable in case you're sure that the models are thread-safe (or implement mutually exclusive access using Dispatch, locks and other trivial technologies if you're not)?

@p2-apple
Copy link
Collaborator

p2-apple commented Aug 22, 2023

This method doesn't need to be actor-isolated because it's not using any actor state, you can just make it a nonisolated func getPatients() ....

Instances of these classes are truly not sendable because they are classes with simple properties that any thread can change. I don't think it's desirable to wrap access to all the properties in some form of mutex. I also don't think they need be sendable, mutability is often desired and ensuring thread-safe access where necessary should be rare?

@ostaptyv
Copy link
Author

...it's not using any actor state...

Correct, it doesn't use any mutable state just yet — this will change in the future. It's just sample code so you can get the surrounding context about where the warning happens.

...ensuring thread-safe access where necessary should be rare

Well, should be. I can imagine a situation where the developer may forget that the FHIRModels are not thread-safe, which then may lead to data corruption if used incorrectly. However, I haven't yet met such a case in my own project, so probably the only thing that bothers me is how to get rid of the warning I mentioned in the original post. One way is to fall back and set the SWIFT_STRICT_CONCURRENCY = minimal in the Build Settings. But maybe there're other ways to do that?

@p2-apple
Copy link
Collaborator

p2-apple commented Sep 1, 2023

Classes with mutable properties inherently are not thread safe. I don't think it makes sense to try to change this in FHIRModels.

@p2-apple p2-apple added wontfix This will not be worked on and removed enhancement New feature or request labels Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants