0

I have the following class for collecting device motion data:

class MotionManager: NSObject {
        static let shared = MotionManager()
        private override init() {}

        // MARK: - Class Variables

        private let motionManager = CMMotionManager()

        fileprivate lazy var locationManager: CLLocationManager = {
                var locationManager = CLLocationManager()
                locationManager.delegate = self
                locationManager.desiredAccuracy = kCLLocationAccuracyBest
                locationManager.activityType = .fitness
                locationManager.distanceFilter = 10.0
                return locationManager
        }()

        private let queue: OperationQueue = {
                let queue = OperationQueue()
                queue.name = "MotionQueue"
                queue.qualityOfService = .utility
                return queue
        }()

        fileprivate var motionDataRecord = MotionDataRecord()

        private var attitudeReferenceFrame: CMAttitudeReferenceFrame = .xTrueNorthZVertical

        var interval: TimeInterval = 0.01
        var startTime: TimeInterval?

        // MARK: - Class Functions

        func start() {
                startTime = Date().timeIntervalSince1970
                startDeviceMotion()
                startAccelerometer()
                startGyroscope()
                startMagnetometer()
                startCoreLocation()
        }

        func startCoreLocation() {
                switch CLLocationManager.authorizationStatus() {
                case .authorizedAlways:
                        locationManager.startUpdatingLocation()
                        locationManager.startUpdatingHeading()
                case .notDetermined:
                        locationManager.requestAlwaysAuthorization()
                case .authorizedWhenInUse, .restricted, .denied:
                        break
                }
        }

        func startAccelerometer() {
                if motionManager.isAccelerometerAvailable {
                        motionManager.accelerometerUpdateInterval = interval
                        motionManager.startAccelerometerUpdates(to: queue) { (data, error) in
                                if error != nil {
                                        log.error("Accelerometer Error: \(error!)")
                                }
                                guard let data = data else { return }
                                self.motionDataRecord.accelerometer = data
                        }
                } else {
                        log.error("The accelerometer is not available")
                }

        }

        func startGyroscope() {
                if motionManager.isGyroAvailable {
                        motionManager.gyroUpdateInterval = interval
                        motionManager.startGyroUpdates(to: queue) { (data, error) in
                                if error != nil {
                                        log.error("Gyroscope Error: \(error!)")
                                }
                                guard let data = data else { return }
                                self.motionDataRecord.gyro = data
                        }
                } else {
                        log.error("The gyroscope is not available")
                }
        }

        func startMagnetometer() {
                if motionManager.isMagnetometerAvailable {
                        motionManager.magnetometerUpdateInterval = interval
                        motionManager.startMagnetometerUpdates(to: queue) { (data, error) in
                                if error != nil {
                                        log.error("Magnetometer Error: \(error!)")
                                }
                                guard let data = data else { return }
                                self.motionDataRecord.magnetometer = data
                        }
                } else {
                        log.error("The magnetometer is not available")
                }
        }

        func startDeviceMotion() {
                if motionManager.isDeviceMotionAvailable {
                        motionManager.deviceMotionUpdateInterval = interval
                        motionManager.startDeviceMotionUpdates(using: attitudeReferenceFrame, to: queue) { (data, error) in
                                if error != nil {
                                        log.error("Device Motion Error: \(error!)")
                                }
                                guard let data = data else { return }
                                self.motionDataRecord.deviceMotion = data
                                self.motionDataRecord.timestamp = Date().timeIntervalSince1970
                                self.handleMotionUpdate()
                        }
                } else {
                        log.error("Device motion is not available")
                }
        }

        func stop() {
                locationManager.stopUpdatingLocation()
                locationManager.stopUpdatingHeading()
                motionManager.stopAccelerometerUpdates()
                motionManager.stopGyroUpdates()
                motionManager.stopMagnetometerUpdates()
                motionManager.stopDeviceMotionUpdates()
        }

        func handleMotionUpdate() {
                print(motionDataRecord)
        }

}

// MARK: - Location Manager Delegate
extension MotionManager: CLLocationManagerDelegate {

        func locationManager(_ manager: CLLocationManager, didChangeAuthorization status: CLAuthorizationStatus) {
                if status == .authorizedAlways || status == .authorizedWhenInUse {
                        locationManager.startUpdatingLocation()
                } else {
                        locationManager.stopUpdatingLocation()
                }
        }

        func locationManager(_ manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
                guard let location = locations.last else { return }
                motionDataRecord.location = location
        }

        func locationManager(_ manager: CLLocationManager, didUpdateHeading newHeading: CLHeading) {
                motionDataRecord.heading = newHeading
        }

}

However I'm getting EXC_BAD_ACCESS after it runs for a while. I ran the zombie instrument and it appears that handleMotionUpdate() is the caller at fault. And MotionDataRecord or some of it's properties are what are being deallocated somehow...

MotionDataRecord is a struct:

struct MotionDataRecord {
    var timestamp: TimeInterval = 0
    var location: CLLocation?
    var heading: CLHeading?
    var motionAttitudeReferenceFrame: CMAttitudeReferenceFrame = .xTrueNorthZVertical
    var deviceMotion: CMDeviceMotion?
    var altimeter: CMAltitudeData?
    var accelerometer: CMAccelerometerData?
    var gyro: CMGyroData?
    var magnetometer: CMMagnetometerData?
}

Any ideas what's going on here?

Edit:

Have added a stripped down version of the project to github here

Edit:

Screenshot of zombies instrument:

zombies instrument screenshot

doovers
  • 8,545
  • 10
  • 42
  • 70
  • @matt No, I don't think so. It seems to be the CoreMotion/Location properties of `MotionDataRecord` that are being deallocated. I'm assuming they are passed by reference so perhaps I need to create my own structs for that data rather than setting those classes to the `MotionDataRecord` struct properties. That seems a bit cumbersome though, is there a better way to get around this? – doovers Dec 09 '16 at 22:08
  • @matt I've added a stripped down version of the project to GitHub (see link at end of question). Would very much appreciate if you could check it out. I'm completely stumped on this one! – doovers Dec 09 '16 at 23:56
  • Do not use static refs to Objective-C or Swift objects. If you need to have a long lived object, hang a ref off of the app delegate and then nil it out when done. This is the root cause of your problem. – MoDJ Dec 10 '16 at 00:08
  • @matt I can't remember, tbh I wrote that a while back but I was probably taking cues from a code snippet I found online. However looking at it now it doesn't seem necessary. I removed all of them but am still getting `EXC_BAD_ACCESS` – doovers Dec 10 '16 at 03:07
  • @matt `MotionLoggerZombieIssue[4535:1787758] *** -[CMGyroData release]: message sent to deallocated instance 0x170c28de0` It's not always `CMGyroData` though – doovers Dec 10 '16 at 03:47
  • @matt Sorry `handleMotionUpdate(data:)` is a relic of the original code I stripped out some stuff to make it as simple as possible. I've just run the zombies instrument again [screenshot](https://dl.dropboxusercontent.com/u/15467419/Screen%20Shot%202016-12-11%20at%2008.55.57.png) – doovers Dec 10 '16 at 21:57
  • @matt Are you suggesting I should do that for test purposes or in general? I could do that but it makes things a bit more cumbersome as I want to be able to pass the data record around. I also want to be able to compare records etc. so bundling it in a struct is much more convenient... I got rid of the `weak self` too! – doovers Dec 10 '16 at 22:06
  • @matt Ok so I've created another branch where I've replaced all the MotionDataRecord CoreMotion and CoreLocation properties with struct equivalents and it seems to have resolved the issue. This is a workable solution, however, it seems like a bit of a hack and I'd really like to understand why the original method is causing problems – doovers Dec 11 '16 at 00:00
  • @matt Yeah I can understand (1) being a factor but do not understand why exactly. Not sure why (2) would be an issue either though! – doovers Dec 11 '16 at 00:12

1 Answers1

2

Okay, I'm going to try to do a little thought-experiment to suggest what might be happening here.

Keep in mind first the following points:

  • Your MotionDataRecord is a struct consisting almost entirely of reference type instance properties. This forces the struct to participate in reference counting.

  • You are wildly accessing the properties of this struct on different threads. Your locationManager:didUpdateLocations: sets motionDataRecord.location on the main thread, while e.g. your motionManager.startDeviceMotionUpdates sets motionDataRecord.deviceMotion on a background thread (queue).

  • Every time you set a struct property, you mutate the struct. But there is actually no such thing as struct mutation in Swift: a struct is a value type. What really happens is that the entire struct is copied and replaced (initializeBufferWithCopyOfBuffer in the zombie log).

Okay, so on multiple simultaneous threads you are coming in and replacing your struct-full-of-references. Each time you do that, one struct copy goes out of existence and another comes into existence. It's a struct-full-of-references, so this involves reference counting.

So suppose the process looks like this:

  1. Make the new struct.

  2. Set the new struct's reference properties to the old struct's reference properties (except for the one we are changing) by copying the references. There is some retain-and-release here but it all balances out.

  3. Set the new struct's reference property that we are replacing. This does a retain on the new value and releases the old value.

  4. Swap the new struct into place.

But none of that is atomic. Thus, those steps can run out of order, interleaved between one another, because (remember) you've got more than one thread accessing the struct at the same time. So imagine that, on another thread, we access the struct between steps and 3 and 4. In particular, between steps 3 and 4 on one thread, we perform steps 1 and 2 on the other thread. At that moment, the old struct is still in place, with its reference to the property that we are replacing pointing to garbage (because it was released and deallocated in step 3 on the first thread). We attempt to do our copy on the garbage property. Crash.

So, in a nutshell, I would suggest (1) make MotionDataRecord a class instead of a struct, and (2) get your threading straightened out (at the very least, get onto the main thread in the CMMotionManager callbacks before you touch the MotionDataRecord).

matt
  • 515,959
  • 87
  • 875
  • 1,141
  • Great explanation. This is starting to make a lot more sense to me now. I have a couple of questions though. 1. If I make `MotionDataRecord` a class and write a function to return a value type copy of the data do I still need to ensure that all access to the class is on the same thread? I'm trying to get my head around why I can't just set the class props from different threads in this scenario. 2. If I do need to dispatch the `MotionDataRecord` updates to a single thread am correct to assume it would be better to use a background thread given how many ops will be done at 100Hz for example? – doovers Dec 11 '16 at 22:13
  • I'd say that the entire 100Hz question is a separate question (you're going to drain the user's battery in no time if you keep that up on _any_ thread). But as for your first question, I would say: take no chances, make no assumptions. Threading is _hard_ and it's _dangerous_. Read Apple's concurrency guide and be very afraid. Then use every safeguard, and confining _all_ access to a shared object to _one_ thread is definitely one of them. Maybe I'm being overly conservative but that's just how I am. – matt Dec 11 '16 at 22:40
  • Ok well that makes sense. I'll definitely read the concurrency guide. Thanks for all your help on this I've learned a lot so it's much appreciated! – doovers Dec 11 '16 at 22:57