hmlongco / Factory

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

When using context runtime arguments in an async context I get unreliable behaviour #137

Closed doozMen closed 11 months ago

doozMen commented 11 months ago

Given

extension Container {
    fileprivate var foo: Factory<String> {
      self { fatalError() }
    }
}

Then I write an async test

  func testAsync() async {
      let key = "option-key"
      FactoryContext.setArg("option 1", forKey: key)

      let options = ["option 1", "option 2", "option 3"]

      for option in options {
        Container.shared.foo.onArg(option) { "foo \(option)" }
      }

      XCTAssertEqual(Container.shared.foo(), "foo option 1")

      for option in options {
        FactoryContext.setArg(option, forKey: key)
        XCTAssertEqual(Container.shared.foo(), "foo \(option)")
      }

      await withTaskGroup(of: Void.self) { group in

        for option in options {
          group.addTask {
            FactoryContext.setArg(option, forKey: key)
            XCTAssertEqual(Container.shared.foo(), "foo \(option)")
          }
        }

        await group.waitForAll()
      }
  }

Which I expect to give the a success but it fails in the async operation XCTAssertEqual failed: ("foo option 2") is not equal to ("foo option 1")

Not sure how to tackle this as I thought based on the locks in your code it would be thread safe?

Could you provide an Actor based solution?

hmlongco commented 11 months ago

Not sure what you're attempting to test, but the following set of actions isn't atomic.

          {
            FactoryContext.setArg(option, forKey: key)
            XCTAssertEqual(Container.shared.foo(), "foo \(option)")
          }

Factory should lock/unlock correctly on the setArg... and then lock/unlock correctly on the resolution. But if you have three operations running asynchronously, then as far as I can tell it's entirely possible to end up with interleaved calls to Factory.

FactoryContext.setArg("option 1", forKey: key) // a1
FactoryContext.setArg("option 2", forKey: key) // a2
XCTAssertEqual(Container.shared.foo(), "foo \(option)") // a1 - ERROR
FactoryContext.setArg("option 3", forKey: key) // a3
XCTAssertEqual(Container.shared.foo(), "foo \(option)") // a2
XCTAssertEqual(Container.shared.foo(), "foo \(option)") // a3
doozMen commented 11 months ago

Yes that is correctly describing my situation. I'm trying to accomplish something like on android that works like compositionLocal Locally scoped data with CompositionLocal  |  Jetpack Compose  |  Android Developers which makes it possible to have a value inside a tree and another in another tree.

CompositionLocal

So I think I abuse the locking system, probably the whole factory system. I can work around the problem by wrapping my work in an actor and that seams to fix the issue.

But what I try to accomplish is how environment in SwiftUI works or composition local in Kotlin. Like you mentioned before not sure if this was the best idea after all.

hmlongco commented 11 months ago

Yeah, it's not an environment variable in a SwiftUI view tree. With Factory, there is no tree. The "arg" mechanism is basically a set of global variables, and the resolution results are dependent on the state of the global variables at any given point in time.

hmlongco commented 11 months ago

The only way to make your code work is to make it atomic as well...

         globalRecursiveLock.withLock {
            FactoryContext.setArg(option, forKey: key)
            XCTAssertEqual(Container.shared.foo(), "foo \(option)")
          }

At which point you might as well have used a ParameterFactory...

          {
            XCTAssertEqual(Container.shared.foo(option), "foo \(option)")
          }
doozMen commented 11 months ago

Yeah well it's not like you did not warn me he. Thanks for clarifying and giving me options. I conclude that this is not the way factory is supposed to work.

For now I fixed the thin a do as you mention with the recursive lock. I will revisit the idea to use the parameter factory at a a later state and made an issue of it in my code. Thanks for the help.