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

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

Open
mihaho opened this issue Dec 1, 2020 · 7 comments
Labels
help wanted Extra attention is needed

Comments

@mihaho
Copy link

mihaho commented Dec 1, 2020

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

  • Xcode Version 12.2 (12B45b)
  • Apple Swift version 5.3.1 (swiftlang-1200.0.41 clang-1200.0.32.8)
  • macOS Big Sur Version 11.0.1

Thanks for your help in advance. I hope that I am not wasting your time by misusing the library 😄

@mbrandonw
Copy link
Member

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
Copy link
Author

mihaho commented Dec 9, 2020

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
Copy link
Member

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
Copy link

rjantz2 commented Dec 30, 2020

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
Copy link
Member

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

@rjantz2
Copy link

rjantz2 commented Dec 30, 2020

so a Combine bug could definitely be the culprit.

Is this Combine bug the one reported here?

Do you have a workaround?

@stephencelis stephencelis transferred this issue from pointfreeco/swift-composable-architecture Jul 7, 2021
@stephencelis
Copy link
Member

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!

@stephencelis stephencelis added the help wanted Extra attention is needed label Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants