RLMNetworkClient leaks NSURLSession

[molind]

!!! MANDATORY TO FILL OUT !!!

Goals

Receive synchronized realm.

Expected Results

Realm is returned. No leaks happened.

Actual Results

RLMNetworkClient leaks NSURLSessions.

Steps to Reproduce

Every call to [RLMNetworkClient sendRequestToEndpoint] leaks one NSURLSession.

Code Sample

Leaking delegate code:

- (void)URLSession:(__unused NSURLSession *)session
              task:(NSURLSessionTask *)task
didCompleteWithError:(NSError *)error
{
    if (error) {
        _completionBlock(error, nil);
        return;
    }

    if (![self validateResponse:task.response data:_data error:&error]) {
        _completionBlock(error, nil);
        return;
    }

    id json = [NSJSONSerialization JSONObjectWithData:_data
                                              options:(NSJSONReadingOptions)0
                                                error:&error];
    if (!json) {
        _completionBlock(error, nil);
        return;
    }
    if (![json isKindOfClass:[NSDictionary class]]) {
        _completionBlock(make_auth_error_bad_response(json), nil);
        return;
    }

    _completionBlock(nil, (NSDictionary *)json);
}

Fixed proof of concept (ugly, but won't leak):

- (void)completeTask:(NSURLSessionTask *)task withError:(NSError *)error {
    if (error) {
        _completionBlock(error, nil);
        return;
    }
    
    if (![self validateResponse:task.response data:_data error:&error]) {
        _completionBlock(error, nil);
        return;
    }
    
    id json = [NSJSONSerialization JSONObjectWithData:_data
                                              options:(NSJSONReadingOptions)0
                                                error:&error];
    if (!json) {
        _completionBlock(error, nil);
        return;
    }
    if (![json isKindOfClass:[NSDictionary class]]) {
        _completionBlock(make_auth_error_bad_response(json), nil);
        return;
    }
    
    _completionBlock(nil, (NSDictionary *)json);
}

- (void)URLSession:(__unused NSURLSession *)session
              task:(NSURLSessionTask *)task
didCompleteWithError:(NSError *)error
{
    [self completeTask:task withError:error];
    _completionBlock = nil;
    [session finishTasksAndInvalidate];
}

Whole concept with new NSURLSession per request seems wrong to me. Because each NSURLSession is ready to process multiple tasks. Why won't create one shared instance of NSURLSession per pinnedCertificatePaths?
Then you could put cert validation into NSURLSessionDelegate as you do now. And json parsing error checking into task completionHandler.
Other option is to invalidate session when task is processed as shown above.

Version of Realm and Tooling

Realm framework version: 3.10.0

Realm Object Server version: 3.4.3

Xcode version: 10.1

iOS/OSX version: iOS 12.1.2

Dependency manager + version:

[bmunkholm]

Thanks a lot for the detailed issue and even suggested fix!
We will take a closer look at it.

[molind]

Actual leaking part is here:
https://github.com/realm/realm-cocoa/blob/56afd03af7f205cee74e9fd359c3f5b7fa502d66/Realm/RLMNetworkClient.mm#L421-L427