pointfreeco / composable-core-location

A library that bridges the Composable Architecture and Core Location.
MIT License
110 stars 56 forks source link

A possible memory leak when composable architecture is combined with the composable core location #8

Open mihaho opened 4 years ago

mihaho commented 4 years ago

Description Hey guys, first of all, I would like to thank you both for making such a great library. Working with it is a lot of fun! πŸ₯‡

I believe, however, that I've found a memory leak when composable architecture is combined with the composable core location dependency in a very specific way. I've prepared a mini demo for you and I would really appreciate it if you'll have time to check it out. It is available here.

To Reproduce

  1. Download the app
  2. Launch the app in the simulator
  3. Allow location updates
  4. Start simulating location updates, by enabling Simulator -> Features -> Location -> City Run
  5. Make sure that you are receiving locations (map is moving)
  6. Leave it running for a second or two, then take a look at the memory graph
  7. You'll see that some of the CLLocation objects are leaking (image attached)

After investigating, I've found out that if I don't combine reducerA into the "main" appReducer, location objects stop leaking. I.e. using just one pulled back child reducer with a core location dependency inside of .combined block works fine. But as soon as you add another "scoped" reducer, the issue reappears. You can reproduce this "fix" by commenting out the following code.

// MARK: - App Reducer

let appReducer = Reducer<AppState, AppAction, AppEnvironment>.combine(

    /*
     * Comment out the following reducer
     */
    reducerA
        .pullback(
            state: \.stateA,
            action: /AppAction.actionA,
            environment: { $0 }
        ),

    reducerB
        .pullback(
            state: \.stateB,
            action: /AppAction.actionB,
            environment: { $0 }
        )
)

The other strange thing that it seems like it also helps is if you remove the map function from the didUpdateLocations method in the core location dependency and only use the last location.

Change from:

func locationManager(_ manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
    subscriber.send(.didUpdateLocations(locations.map(Location.init(rawValue:))))
}

to:

func locationManager(_ manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
    subscriber.send(.didUpdateLocations(Location.init(rawValue: locations.last!)))
 }

Expected behavior CLLocation objects should not leak.

Screenshots

MemoryGraph

Environment

Thanks for your help in advance. I hope that I am not wasting your time by misusing the library πŸ˜„

mbrandonw commented 3 years ago

Thanks for the report, I was able to repro this and I think your example is doing everything just fine. I don't exactly see how anything could be leaking, and unfortunately the memory debugger (and even the leaks instrument) isn't giving much helpful information.

I'll try to investigate more, but I'm a bit stumped right now 😟

mihaho commented 3 years ago

Thanks for taking the time and checking out the demo. I also don't know what's going on, it doesn't make any sense. As I said above, we removed the map function from Composable's LocationManager didUpdateLocation and refactored the code to use only one (last) location. It fixed the issue but I do not understand how could this help πŸ€·β€β™‚οΈ.

I forgot to mention one more thing. We are using a 3rd party framework which internally starts monitoring regions and as a result, the composable core location dependency is notified about those events too. Unfortunately, CLRegion objects start leaking too via didEnter and didExit methods, just like CLLocation objects.

Anyways, thanks again πŸ™ . I'll let you know if I find anything out.

mbrandonw commented 3 years ago

Yeah, the fact that not combining reducers changes this makes me think it's actually secretly a Combine leak. We've seen this before: pointfreeco/swift-composable-architecture#195. I believe this has been fixed now, but up until Xcode 12.x if you concatenated a MergeMany you would get a leak. Maybe something like that is going again :/

rjantz2 commented 3 years ago

Is merging the same as concatenating? In Xcode 12.3 I'm having memory leaks when using Reducer.combine in the main app reducer. The number of memory leaks correlates to the number of reducers combined.

stephencelis commented 3 years ago

@rjantz2 Reducer.combine merges effects under the hood, so a Combine bug could definitely be the culprit.

rjantz2 commented 3 years ago

so a Combine bug could definitely be the culprit.

Is this Combine bug the one reported here?

Do you have a workaround?

stephencelis commented 3 years ago

Since this is related to composable-core-location I've transferred the issue here. I think one potential fix would be for Location to not hold onto CLLocation, though this might not be feasible depending on APIs that take CLLocations, but if anyone has time to dive in, that would be a good place to start!