hmlongco / Factory

A new approach to Container-Based Dependency Injection for Swift and SwiftUI.
MIT License
1.83k stars 115 forks source link

Sendable conformance for `Container` #190

Closed erichoracek closed 3 months ago

erichoracek commented 6 months ago

With Swift 5.10 released and Swift 6 on the horizon, consumers of Factory would expect Container to conform to Sendable as it is intended to be thread safe. However, since it contains a property of containerManager, it's not possible to mark it as such:

image

Furthermore, when you attempt to conform ContainerManger to sendable, you notice that it has publicly writable properties that are not protected, namely defaultScope but also some other debug ones, so it cannot conform to Sendable either:

image

To satisfy the Sendable contract, ideally Container and ContainerManager would both have Sendable conformances, though this might require an API rethink.

hmlongco commented 6 months ago

Most of Factory is locked and already thread-safe, so adding complete sendable conformance shouldn't be that difficult.

doozMen commented 6 months ago

The FactoryContext is not thread-safe. It accesses and mutates a struct

extension FactoryContext {
    /// Add argument to global context.
    public static func setArg(_ arg: String, forKey key: String) {
        FactoryContext.current.runtimeArguments[key] = arg
    }
    /// Add argument to global context.
    public static func removeArg(forKey key: String) {
        FactoryContext.current.runtimeArguments.removeValue(forKey: key)
    }
}

This leaves me to think that a lot needs to change in order to go for Swift 6 and sendable checks. Do you think it is feasible to refactor factory to conform to all that?

emadhegab commented 5 months ago

is there any intension to fix this?

NachoSoto commented 3 months ago

I just sent #209 so at least Factory doesn't fail to compile with Swift 6.

hmlongco commented 3 months ago

See the Swift6 branch. That's Sendable and compatible with Swift concurrency.