PhilipsHue / PhilipsHueSDK-iOS-OSX

The Software Development Kit for Philips Hue on iOS and OS X (beta)
579 stars 169 forks source link

Critical Leak : PHBridgeSendAPI - (PHRequest *)updateLightStateForId:(NSString *)lightIdentifier withLightState:(PHLightState *)lightState completionHandler:(PHBridgeSendErrorArrayCompletionHandler)completionHandler; #75

Closed false closed 8 years ago

false commented 9 years ago

If you call that method in a loop you can clearly see through Xcode Instruments Allocation tools that there is a leak with the requests (CFNetwork, URLResponse, URLRequest ...).

It leads to the crash of my application on the middle term (10-20 minutes) as I call the method quite often.

Could you fix the leak, as we don't have access to the SDK sources?

Quick Exemple : UIColor* color = [UIColor whiteColor]; for(int i=0; i<100000; i++) { CGPoint xy = [PHUtilities calculateXY:color forModel:light.modelNumber];

                PHLightState *lightState = [[PHLightState alloc] init];
                lightState.x = @(xy.x);
                lightState.y = @(xy.y);

                [self.bridgeSendAPI updateLightStateForId:light.identifier withLightState:lightState completionHandler:^(NSArray *errors) {
                    if (errors != nil) {
                        NSString *message = [NSString stringWithFormat:@"%@: %@", NSLocalizedString(@"Errors", @""), errors != nil ? errors : NSLocalizedString(@"none", @"")];

                        NSLog(@"Response: %@",message);
                    }
                }];

}

false commented 9 years ago

I may have found a way to fix it (until it's fixed in the SDK)

false commented 9 years ago

My solution just helps reducing the calls, thus the leak ; But the leak is still there. I can't release my application because of this leak. My application gets to 100+ Mbits of memory use after 20 minutes.

Could you please have a look @pverh ? Thanks

pverh commented 9 years ago

Hi, looks like a valid issue. Thanks for reporting. Unfortunately I cannot give you an estimation yet on when this will be fixed. I'll try to report back to you on this asap.

monoxygen commented 9 years ago

I've noticed the same leak and very bad memory growth. My workaround was also to dispatch_async to the main thread. updateLightStateforID:withLightState: is abandoning a 132 KB malloc object every time it is called, so call it 10 times and you've already abandoned over 1 MB of memory. Instruments shows the object being allocated in a CFReadStream callback scheduled on the main run loop by CFNetwork.

bmaltais commented 9 years ago

This is what is causing my app to crash after a couple of days! I have been looking at the memory consumption when it is running and it is an hungry one! screen shot 2014-11-30 at 2 07 30 pm

bmaltais commented 9 years ago

And calling using dispatch_async is not much better..

screen shot 2014-11-30 at 2 13 40 pm

This is how I am calling it:

dispatch_async(dispatch_get_main_queue(), ^(void){ // Send lightstate to light [bridgeSendAPI updateLightStateForId:light.identifier withLightState:lightState completionHandler:^(NSArray errors) { if (errors != nil) { NSString message = [NSString stringWithFormat:@"%@: %@", NSLocalizedString(@"Errors", @""), errors != nil ? errors : NSLocalizedString(@"none", @"")];

                                       NSLog(@"Response: %@",message);
                                   }
                               }];
                           });
bmaltais commented 9 years ago

Would be nice if Philips made the framework source code available so we could fix this on our own ;-(

monoxygen commented 9 years ago

@bmaltais you might be seeing memory growth when running in Xcode due to 'Performance tool data' growth. I'm not sure what's triggering it (seems like something Xcode is allocating on your behalf for memory analysis), but as long as I use dispatch_async to the main thread AND run outside of Xcode, memory growth seems to stay constant.

bmaltais commented 9 years ago

OK, let me give it a try outside xcode then.

false commented 9 years ago

Any update?

ibuprofane commented 9 years ago

I'm encountering this same issue. Quick resolution would be much appreciated!

lightbow commented 9 years ago

I believe Philips just needs to change their code from using NSURLConnection to using NSURLSession. From much experimentation, there's no other way to avoid the memory leak. While the old way should be fine for many apps, Lightbow's animations sometimes shoot out hundreds of light state changes, and with the memory leak down in iOS with no viable workarounds I could find, I was forced to write my own light-state-dispatch code using the more modern NSURLSession. @pverh I would love to ditch my code and switch back to using the Philips SDK for this! For those of you needing a solution while we wait for a fix in the SDK, here's some Swift code that might be helpful, showing how to easily turn a json dictionary into a NSURLRequest/NSURLSession. (ignore the LB-specific stuff... this is just to hopefully save folks some time in figuring out the specifics of some of the constants)

func submitJSON(json: AnyObject, post: Bool, path: String, successHandler: ((NSURLResponse!,NSData)->Void)?, failureHandler: (()->Void)?)
    {
        var error: NSError?
        let data = NSJSONSerialization.dataWithJSONObject(json, options: nil, error: &error)
        LBErrorAndWarningController.sharedController().reportError(error)

        if error != nil {
            failureHandler?()
            return
        }

        let url = NSURL(string: path)
        let request = NSMutableURLRequest(URL: url!)

        request.HTTPMethod = post ? "POST" : "PUT"
        request.setValue("application/json", forHTTPHeaderField: "Accept")
        request.setValue("application/json", forHTTPHeaderField: "Content-Type")
        request.cachePolicy = .ReloadIgnoringLocalCacheData
        request.setValue("\(data?.length)", forHTTPHeaderField: "Content-Length")
        request.HTTPBody = data

        let task = NSURLSession.sharedSession().dataTaskWithRequest(request, completionHandler: { (responseData, response, connectionError) -> Void in

            LBErrorAndWarningController.sharedController().reportError(connectionError)

            if connectionError == nil {
                successHandler?(response, responseData)
            } else {
                failureHandler?()
            }
        })
        task.resume()
}
false commented 9 years ago

Hello pverh,

You should reproduce the leak with that piece of code :

UIColor* color = [UIColor whiteColor]; for(int i=0; i<100000; i++) { CGPoint xy = [PHUtilities calculateXY:color forModel:light.modelNumber];

            PHLightState *lightState = [[PHLightState alloc] init];
            lightState.x = @(xy.x);
            lightState.y = @(xy.y);

            [self.bridgeSendAPI

updateLightStateForId:light.identifier withLightState:lightState completionHandler:^(NSArray errors) { if (errors != nil) { NSString message = [NSString stringWithFormat:@"%@: %@", NSLocalizedString(@"Errors", @""), errors != nil ? errors : NSLocalizedString(@"none", @"")];

                    NSLog(@"Response: %@",message);
                }
            }];

}

Best,

On 15 April 2015 at 15:32, pverh notifications@github.com wrote:

Sorry for the late response. We are currently investigating this issue. Could some of you please provide a minimal piece of reproduction code in which they experience the leak? Thanks!

— Reply to this email directly or view it on GitHub https://github.com/PhilipsHue/PhilipsHueSDK-iOS-OSX/issues/75#issuecomment-93400086 .

pverh commented 9 years ago

@false I'm currently investigating this issue and tried your example code in the QuickStart app and it doesn't appear to be leaking after warming up when setting the counter to 500 and performing it repeatedly. It is true that while performing lots of request in a loop the memory footprint rises rapidly but this is only because the memory used by the http requests will not be released until the requests finish (timeout/fail or have a successful response). When firing more request then the number of request that finish the memory footprint starts to increase.

@macmantrl @bmaltais Do you still have these issues? could you share some example code as well?

false commented 9 years ago

@pverh The thing is the memory leak should not happen. In the case of my application that sends about 4 requests every seconds, the application freezes after 10 - 15 minutes. And if I set the limit it to 1 request every second, the problem still occurs 30-40 minutes.

On the application side, we can't control the fact that too many requests are still in suspend ; And in the case of a "big" loop the framework should queue the requests ( and/or ignore some of them?).

Do you use the NSURLSession? Do you queue the requests?

lightbow commented 9 years ago

I also recommend making the switch to the more modern NSURLSession. The older NSURLConnection stuff has some long-standing leaks (even with some of the cache-clearing hacks you can find on stackoverflow) and it looks like Apple isn't planning on fixing them since NSURLSession is the future. Here's some more background information: http://www.objc.io/issue-5/from-nsurlconnection-to-nsurlsession.html

pverh commented 9 years ago

@false Unfortunately we are currently not queueing requests, they are executed right away. If you want a queue you will have to implement it yourself. We also don't use NSURLSession at this moment.

I agree with you that our way of handling requests is not what it should be. We are looking into refactoring our connection logic and using NSURLSession, but as the impact is considered high this will not make it in the upcoming SDK release.

I have spotted some issues with the current implementation that I would like to share:

lightbow commented 9 years ago

Light commands spaced out at 0.1s already seemed like a frustrating limitation, and limits what we can do with light animations, especially on many-light setups. It would be a huge problem to increase this to 0.5s (two calls per second). Lightbow currently uses its own custom Swift / NSURLSession-based light state dispatch and caching system, but I wish I could toss it out and use the standard Philips one in your SDK one day. (I originally wrote my own to avoid the leak that's the subject of this thread)

false commented 9 years ago

Any update on the leak issue?

jhvdb87 commented 8 years ago

We switched to NSURLSession in the 1.11 release and we also did a memory leak fix mentioned by pverh.