Store dependencies default values on first access

[tgrapperon]

This attempts to mitigate issues similar to #1508, where it is easy to assume that some dependency's default value is only set once whereas a new value is silently generated each time the dependency is accessed. To prevent this potentially surprising behavior, the default value is saved into some global storage on first access, and reused on subsequent accesses.

Because a dependency can be accessed from any concurrent context, this global storage is accessed under locking. The complete caching operation result on a relatively small overhead:

name                                 time       std        iterations
---------------------------------------------------------------------
Read default `liveValue` (current)   625.000 ns ±  59.29 %    1000000
Read default `liveValue` (proposed)  750.000 ns ±  54.80 %    1000000

This 0.1µs increase shouldn't be noticeable in real-life applications.

[tgrapperon]

Oops, I thought that NSRecursiveLock.withLock { … } was the closure version shipping with TCA whereas it's in reality the new shinny macOS 13 built-in one.

[iampatbrown]

@tgrapperon I've pushed a branch which is similar to how I've been doing it. It's probably got a bit more overhead than yours but I needed more control over the global defaults.

https://github.com/pointfreeco/swift-composable-architecture/compare/main...iampatbrown:swift-composable-architecture:shared-dependencies

[tgrapperon]

@iampatbrown Nice! For some reason, I wanted to deport caching closer to DependencyKey than to DependencyValues, but I can make some changes to bring it in like you did if one finds it better. As an aside, and you of curiosity, did you had some real-life uses of crossed dependencies like in your tests?

[iampatbrown]

I wanted to deport caching closer to DependencyKey than to DependencyValues

Yeah that makes sense. I mainly needed global dependencies so that's why I took that approach. I wasn't suggesting you change your implementation :)

did you have some real-life uses of crossed dependencies like in your tests

Yes. The cross dependencies existed before I started on the project though. It wouldn't be something I'd really need myself.

[tgrapperon]

Here is a variant where successive values are compared in DEBUG. In case of discrepancy, a runtime warning is presented. According to this tweet, it does the same as SwiftUI and starts with memcmp, and falls back to Equatable if possible. It likely produces false positives with dependencies having a function as value. It could be interesting to check in real-life projects.

[stephencelis]

Thanks for looking into this! @mbrandonw and I chatted about it today and we think the main thing missing would be to tie the storage to DependencyValues instead of a global variable, as that would by default enforce better test separation.

@iampatbrown's branch looks like a good step in that direction. How do you feel about refining the solution in kind?

(Side-note to @iampatbrown: nice @Isolated wrapper 😄 ...we've talked about shipping a @LockIsolated helper but haven't committed to it yet.)

[tgrapperon]

@stephencelis I didn't spot that TestReducer was creating its own DependencyValues. That makes sense indeed.

I've updated the PR with a potential implementation in this direction. I went the one-dictionary route, but it can be three like Pat's implementation, with a shared lock or three. ~I find explicit implementations clearer than using a common utility function, as the specific treatment for .liveValue would make its signature overly complicated.~ [Edit: Replaced by an helper function in a latter commit]

It also seems that one could reuse the ObjectIdentifier(key), but it's probably premature optimization, as creating instances of this thing is usually really fast. But I can make the change and reuse the ObjectIdentifier value if you want.

Please let me know what you think.

[tgrapperon]

I've just benchmarked reusing ObjectIdentifier(key) values, and it doesn't result in any measurable gain.

[tgrapperon]

@stephencelis I've slightly simplified the implementation and renamed a few things. I think that's good for me. Please let me know what you think.

[mbrandonw]

Hi @tgrapperon, I just pushed 2f4d842...93d3af9 with some changes that came from trying to write tests for the subtle logic in the PR and realizing things aren't quite what the seem.

• First, I made a few cosmetic updates, some related to the PR and some not. For example, I grouped some things together I've been meaning to do, and I renamed some things.
• There's a small problem in the withValue/withValues functions on main where we should flip isSetting back to false once you are inside the operation. People using TCA aren't likely to run into this, but once we extract it to a separate repo it will be easier to run into problems.
• I pushed the logic of detecting accessing test dependencies in a live context down in the cached values object.
• We will not cache the access of a test dependency in a live context since an erroneous situation anyway.
• I refactored the test to use a client that manages some mutable private state so that we could more easily see that the state doesn't get reset every time the dependency is made.
• I wrote a new test to show that the cached values are segmented by context.

I think now we are comfortable merging this.

One thing to note is that by putting the cached reference value in DependencyValues we are making it possible for dependencies to bleed across uses, which isn't possible on main. This manifested itself in a real way in tests where I needed to do withValue(\.self, .init()) { ... } in order to start with a clean slate.

This doesn't really affect TCA since every TestStore starts with a fresh DependencyValues, but it is something to think about when/if we decide to extract into a separate library.

[mbrandonw]

Pushed one last commit bf17531190 to update docs and update places we were using the { ... }() pattern.

[mbrandonw]

@tgrapperon Thinking we'll merge soon, but want to make sure you are OK with all the changes we made.

[tgrapperon]

No problem. Beside unaccounted edge effects, I tried to see if there were cases where one legitimately wouldn't want this behavior, but I haven't found anything credible.

[tgrapperon]

Alternatively, if someone encounters a problematic configuration, it seems possible to expand TestDependencyKey with a new property requirement that would allow to opt-out of this behavior.

[csanfilippo]

Hi @stephencelis and @tgrapperon, I'm using the Dependencies target outside the Composable Architecture and I would like to find a way to not cache every dependency I'm using in my app. In particular, if I'm declaring a dependency like this

private enum CurrentDateDependencyKey: DependencyKey {
    static var liveValue: Date {
        return Date()
    }
}

public extension DependencyValues {
    var currentDate: Date {
        get { self[CurrentDateDependencyKey.self] }
        set { self[CurrentDateDependencyKey.self] = newValue }
    }
}

I expect that every time I inject the current date using the Dependency property wrapper, the current date is returned, not a cached one.

@Dependency(\.currentDate) var date1: Date

// after some time

@Dependency(\.currentDate) var date2: Date

I expect that date1 != date2

How would you achieve the result I'm expecting with the code above?

Thanks!

[tgrapperon]

Hey @csanfilippo! I think that you would want your dependency to vend a () -> Date function that you call on demand (like the built-in date dependency does (using a struct with callAsFunction, but the idea is the same)) So with the build-in dependency, you have

@Dependency(\.date) var date1
@Dependency(\.date) var date2
date1() != date2()
date1() != date1()
// or
@Dependency(\.date.now) var now1
@Dependency(\.date.now) var now2
now1 != now2
now1 == now1 // (of course)

But you can apply the idea of returning "generators of values" (as functions or "callable" structs) to general dependencies other than date. It works the same for uuid for example.

[csanfilippo]

Thanks @tgrapperon! Your answer produces another question :). Would it make sense to build the concept of "lifetime" (I don't know how to express it better) in the library itself? I'm thinking of a way to explicitly declare if the object should be recreated each time you inject it or it should be something allocated just once. Does it make sense to you?

[tgrapperon]

@csanfilippo, I think this is already achievable, as you can override any dependency from a parent reducer using the .dependency(\.date, …) modifier. You can then choose to always send the same value, or a new one each time. There were discussions about a StateReader reducer that would work similarly to a GeometryReader in SwiftUI, and allow to pick up some data that you're persisting in the state to inject it back into the dependency chain. As you explicitly set the data for each action received, there is no "caching/capture" issues. This PR was mostly an implementation detail to avoid a common mistake when reference types are involved, but if you have sample case with the concept of "lifetime", feel free to open a thread in the Discussions section, so we can discuss more swiftly!