Skip to content
This repository has been archived by the owner on Oct 6, 2021. It is now read-only.

Commit

Permalink
proposed fix for github issue foursquare#8: error property now set on…
Browse files Browse the repository at this point in the history 
… main thread; complete is idempotent by ignoring subsequent calls.
  • Loading branch information
@gwk
gwk committed Jan 9, 2014
1 parent 6422b5f commit 9cbe3d4
Showing 1 changed file with 18 additions and 28 deletions.
46 changes: 18 additions & 28 deletions src/FSNConnection.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ - (void)connectionDidFinishLoading:(NSURLConnection *)connection {
[self callOrDispatchParse];
}
else {
[self performComplete];
[self performCompleteWithError:nil];
}
}

Expand All @@ -257,7 +257,7 @@ - (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)err
FSNVerbose(@"%p: didFail", self);
self.finishOrFailInterval = [self intervalSinceStart];
[self.responseStream close];
[self failWithError:error];
[self performCompleteWithError:error];
}


Expand Down Expand Up @@ -459,37 +459,29 @@ - (void)parse {
self.parseResult = self.parseBlock(self, &error);
self.parseInterval = [self intervalSinceStart];

if (error) {
[self failWithError:error];
}
else {
[self performComplete];
}
[self performCompleteWithError:error];
}

[self.blocksLock unlock];
}


- (void)failWithError:(NSError *)error {
FSNLog(@"failWithError: %@\n error: %@", self, error);
NSAssert(!self.error, @"error already set");
self.error = error;

[self performComplete];
- (void)performCompleteWithError:(NSError *)error {
[self performSelectorOnMainThread:@selector(completeWithError:) withObject:error waitUntilDone:NO]; // call retains self
}


- (void)performComplete {
FSNVerbose(@"%p: performComplete", self);
self.connection = nil;
[self performSelectorOnMainThread:@selector(complete) withObject:nil waitUntilDone:NO]; // call retains self
}


- (void)complete {
- (void)completeWithError:(NSError *)error {
FSNVerbose(@"%p: completeWithError: %@", self, error);
ASSERT_MAIN_THREAD;

// this may get called twice due to didExpireInBackground.
if (self.didComplete) { // second call
FSNLogError(@"completeWithError: %@\n previous error: %@", error, self.error);
return;
}
NSAssert(!self.error, @"error already set: %@", self.error); // error should not be set anywhere else.
self.error = error;
self.connection = nil;
self.didComplete = YES;

[self reportProgress];
Expand Down Expand Up @@ -540,7 +532,7 @@ - (FSNConnection *)start {
NSURLRequest *urlRequest = [self makeNSURLRequest];

if (![NSURLConnection canHandleRequest:urlRequest]) {
[self failWithError:
[self performCompleteWithError:
[NSError errorWithDomain:FSNConnectionErrorDomain
code:0
userInfo:[NSDictionary dictionaryWithObject:@"request cannot be handled"
Expand All @@ -555,8 +547,7 @@ - (FSNConnection *)start {
startImmediately:NO];

if (!self.connection) {

[self failWithError:
[self performCompleteWithError:
[NSError errorWithDomain:FSNConnectionErrorDomain
code:0
userInfo:[NSDictionary dictionaryWithObject:@"could not establish connection"
Expand Down Expand Up @@ -607,11 +598,10 @@ - (void)didExpireInBackground {

[self cancelConnection]; // releases delegate (self)

[self failWithError:
[self completeWithError:
[NSError errorWithDomain:FSNConnectionErrorDomain
code:FSNConnectionErrorCodeExpiredInBackgroundTask
userInfo:[NSDictionary dictionaryWithObject:@"expired in background task" forKey:@"description"]]];

}

#endif
Expand Down

3 comments on commit 9cbe3d4

@gwk
Copy link
Owner Author

@gwk gwk commented on 9cbe3d4 Jan 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gets rid of failWithError, which was an intermediate step that occurs in the background. it also moves the connection nullification from performCompleteWithError to completeWithError; callers from the main thread can just call the latter directly. As I recall, the old idea was that we want to release things as soon as possible, but in hindsight it seems safer and clearer to just do it on the main thread a tiny bit later.

all of these state updates now happen in completeWithError, which is made idempotent by a check to self.didComplete.

i have not tested this beyond running the demo app and i understand that this might be a higher risk patch than you are willing to deal with at the moment, but i do think it's a step in the right direction for the library. it would be great if someone could put this into a beta build and qualify it.

@braintreeps
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a huge amount of empirical evidence to back it, this looks like a reasonable change that would help with race conditions around failWithError: and setError:.

Thanks @gwk for the quick maintenance work. Do you maintain this fork? We'd consider using your code if it tackles a few bugs foursquare isn't ready to deal with at the moment.

@gwk
Copy link
Owner Author

@gwk gwk commented on 9cbe3d4 Jan 14, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.