views:

184

answers:

6

Hi all,

I am creating instances of a class FlickrImage parsing a Flickr API photos response. The class has a method getLocation that does another API call to get the geolocation:

NSLog(@"getting location for %i",self.ID);
NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
OFFlickrAPIRequest *flickrAPIRequest = [[OFFlickrAPIRequest alloc] initWithAPIContext[appDelegate sharedDelegate].flickrAPIContext];
[flickrAPIRequest setDelegate:self];

NSString *flickrAPIMethodToCall = @"flickr.photos.geo.getLocation";
NSDictionary *requestArguments = [[NSDictionary alloc] initWithObjectsAndKeys:FLICKR_API_KEY,@"api_key",self.ID,@"photo_id",nil];

[flickrAPIRequest callAPIMethodWithGET:flickrAPIMethodToCall arguments:requestArguments];
[pool release];

I have implemented the callback method that would catch the response from the API and update the FlickrImage instance with the geolocation data - but it never gets called. Here's where the instances get created:

NSDictionary *photosDictionary = [inResponseDictionary valueForKeyPath:@"photos.photo"];
NSDictionary *photoDictionary;
FlickrImage *flickrImage;
for (photoDictionary in photosDictionary) {
 flickrImage = [[FlickrImage alloc] init];
 flickrImage.thumbnailURL = [[appDelegate sharedDelegate].flickrAPIContext photoSourceURLFromDictionary:photoDictionary size:OFFlickrThumbnailSize];
 flickrImage.hasLocation = TRUE; // TODO this is actually to be determined...
 flickrImage.ID = [NSString stringWithFormat:@"%@",[photoDictionary valueForKeyPath:@"id"]];
 flickrImage.owner = [photoDictionary valueForKeyPath:@"owner"];
 flickrImage.title = [photoDictionary valueForKeyPath:@"title"];
 [self.flickrImages addObject:[flickrImage retain]];
 [flickrImage release];
 [photoDictionary release];
}

The retain is there because I thought it might help solve this but it doesn't - and doesn't the NSMutableArray (flickrImages is a NSMutableArray) retain its members anyway?

EDIT I should add that the getLocation method (first code snippet) is launched in a thread: [NSThread detachNewThreadSelector:@selector(getLocation) toTarget:self withObject:nil];

A: 

I'm not familiar with these flicker API wrappers, but in this code:

NSDictionary *requestArguments = [[NSDictionary alloc] initWithObjectsAndKeys:FLICKR_API_KEY,@"api_key",self.ID,@"photo_id",nil];

Are you certain that both FLICKR_API_KEY, and self.ID are not nil? If either of them is nil, you'll end up with a dictionary that has less items in it than you intend.

Jon Hess
Jon - thanks, I'm pretty sure they aren't - although I am still struggling with variable watching in Xcode and end up putting a lot of NSLog() statements in my code. I'll double check though.
mvexel
Just did and they are in fact non-nil.Last I get in the GDB output is [Switching to process 87776] Program received signal: “EXC_BAD_ACCESS”. [Switching to process 87776]which makes me think that the callback for the asynchronous API request is sent to something that does not exist anymore.
mvexel
I call the getLocation directly now and the callback gets called now. Threading was not necessary in this case because the ObjectiveFlickr API is asynchronous anyway. Running into different trouble now, will follow up in different thread, thanks for this.
mvexel
A: 

The setDelegate method of OFFlickrAPIRequest does not retain the delegate like it should. This means you're stuck ensuring that your delegate is alive as long as the request is (or patching the class to properly own its own references).

jon hohle
This sounds like it could be the correct answere, but whe you say "dies not retain the delegate like it should", that's not quite right. Delegates normally aren't retained, and that's on purpose. If a table view for example retained it's delegate, a UITableViewController, then there would be a retain cycle because the controller is also retaining the table view. Delegates are pretty much always the owner in these relationships so the delegating reference is non-retaining. This is of course all much simpler with garbage collection, on Mac OS X.
Jon Hess
That makes sense, as you'd likely get a circular reference if the delegate was retained.
jon hohle
+1  A: 

I've also run into this problem when requesting and parsing XML on a different thread my solution was to do this:

while([[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:start] && !isFinished){

}

Where start = [NSDate dateWithTimeIntervalSinceNow:3]; this is basically a timeout so that it doesn't live forever and isFinished is set to true when my parsing has completed.

James Raybould
This is a valid solution, but if the requesting and parsing all happens via the run loop there's no need to have it on a separate thread in the first place.
benzado
A: 

Could you post the callback method(s) you have implemented – this could be just down to a simple typo, as it appears OFFlickrAPIRequest won’t do anything if the delegate does not implement the required callback.

Did you also implement flickrAPIRequest:didFailWithError: to see if there was an error returned from the API call?

Ciarán Walsh
+2  A: 

Your delegate method is never being called because the request is never being made. When you call callAPIMethodWithGET:, it sets up communications to run asynchronously on the current thread's run loop, then returns immediately. That way you can safely call it on the main thread without blocking.

Because you are calling the method from a thread you created yourself, it does not see the main run loop, but the run loop for your new thread. However, because you never execute the run loop, the messages are never sent, a response is never received, and your delegate is never called.

You could fix this by calling [[NSRunLoop currentRunLoop] run] in your new thread. That will let the work happen. But in this case would be easier to never detach a new thread in the first place. Your program won't block, and you won't have to worry about your delegate method needing to be reentrant.

benzado
A: 

Okay, I did solve it, with help from some of the suggestions above.

  • I did remove the extra retain because it did in fact create a memory leak. It did not look right from the outset, so my gut feeling about that is worth something, which is a good thing ;)
  • I removed the redundant threading because the API call is already asynchronous and does not require an additional thread to be non-blocking. After that, the callback method was being called but I ran into different problems concerning object retention. If interested you might want to check out that question, too

Thanks all.

mvexel