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

Fix: check if authCallback is running on a background queue #536

Merged
merged 2 commits into from Nov 22, 2016
Merged

Conversation

ricardopereira
Copy link
Contributor

@ricardopereira ricardopereira commented Nov 17, 2016

#531

  • interacting with certain frameworks like UIKit, we need to ensure that all calls happen from the main thread.

 - interacting with certain frameworks like UIKit, we need to ensure that all calls happen from the main thread.
@mattheworiordan
Copy link
Member

@ricardopereira does this PR fix #531?

@ricardopereira
Copy link
Contributor Author

@mattheworiordan Yes. I reproduced the issue and now it's working.

@ricardopereira
Copy link
Contributor Author

BTW, for the record:

@implementation AppDelegate {
    ARTClientOptions *_clientOptions;
    ARTRealtime *_client;
}

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
    _clientOptions = [[ARTClientOptions alloc] init];
    _clientOptions.authCallback = ^(ARTTokenParams *params, void(^callback)(id<ARTTokenDetailsCompatible>, NSError*)) {
        callback(nil, [NSError errorWithDomain:@"localhost" code:0 userInfo:nil]);
    };
    _client = [[ARTRealtime alloc] initWithOptions:_clientOptions];
    [_client.connection on:^(ARTConnectionStateChange *stateChange) {
        NSLog(@"%@", stateChange);
    }];
    return YES;
}

@end

The callback(nil, [NSError errorWithDomain:@"localhost" code:0 userInfo:nil]); was not called.

@tcard
Copy link
Contributor

tcard commented Nov 17, 2016

I'm not sure this solves the problem as generally as it could. What's so special about authCallback? Shouldn't this be done deeper, like at the ARTHttp level? Or all the places where the CFRunLoopPerformBlock thing is used?

@ricardopereira
Copy link
Contributor Author

I'm not sure this solves the problem as generally as it could.

You're right.

What's so special about authCallback?

I don't have an answer. What I know is that the application:didFinishLaunchingWithOptions is running in the main queue and you can test it quite easily (just create a UIView and present it... it works). I debug it line by line and the only case where the execution transitions to a background queue is when the HTTP request is made. The completion is called in the background. Maybe because the authCallback is created in the main queue.

@tcard After I opened the PR I started thinking on generalising the solution. I though checking every callback that is accessible by the user (like, authCallback, state changes, publish message callback) and add the code to execute the callback on the main queue if needed. What do you think?

@tcard
Copy link
Contributor

tcard commented Nov 17, 2016

I though checking every callback that is accessible by the user (like, authCallback, state changes, publish message callback) and add the code to execute the callback on the main queue if needed. What do you think?

Sure, sounds better to me. Although honestly I'm flying a bit blind here, I don't really know how GCD works, etc. so you'll probably know better.

Just please let's avoid duplicating code as we do here, we can put this if-else logic in a generic function that takes a block.

@tcard
Copy link
Contributor

tcard commented Nov 22, 2016

@ricardopereira Is there anything missing here?

@ricardopereira
Copy link
Contributor Author

@tcard No, it's done.

@tcard
Copy link
Contributor

tcard commented Nov 22, 2016

@ricardopereira Cool, thanks. I still don't get what's special about authCallback, but well...

@tcard tcard merged commit 17c40cc into master Nov 22, 2016
@tcard tcard deleted the fix-531 branch November 22, 2016 19:13
@ricardopereira
Copy link
Contributor Author

@tcard I explained here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants