Retention of arguments

Discussion of the OCMock framework. If you have patches we would prefer you to send them to the mailing list, but attaching them to a topic is possible, too.

Retention of arguments

Postby danyowdee » 01 Oct 2015, 12:28

Admittedly I’ve only skimmed the forums here, but didn’t find anything in this regard:

After seeing some dubious effects in my tests, I browsed through the source code of OCMock and now have the strong suspicion that verifying certain interactions leads to leaking memory all over the place.

Consider the following setup:
Code: Select all
NS_ASSUME_NONNULL_BEGIN

@protocol MessengerDelegate;

//////////////////////
@interface Messenger : NSObject

@property (weak, nonatomic) id<MessengerDelegate> delegate;
- (BOOL)sendMessage:(NSString *)message error:(NSError **)error;

@end

//////////////////////
@protocol MessengerDelegate <NSObject>

- (BOOL)messenger:(Messenger *)messenger canSendMessage:(NSString *)message;

@end

NS_ASSUME_NONNULL_END


If I want to test that the messenger consults its delegate before attempting to send a message, the test would probably look like this:

Code: Select all
// Assuming ARC in Objective-C++
#define ProtocolMock(protocolName) (id<protocolName>)OCMProtocolMock(@protocol(protocolName))

- (void)testThatMessengerRespectsDelegateDecisionWhenSendingMessage {
    auto const subject = [Messenger new];
 
    auto const delegate = ProtocolMock(MessengerDelegate);
    // Even though the nice mock will return NO implicitly, let’s be _explicit_ about it, and stub:
    OCMStub([delegate messenger:[OCMArg any] canSendMessage:[OCMArg any]]).andReturn(NO);
    subject.delegate = delegate;

    // use a string that can be released
    auto const message = [[NSString alloc] initWithFormat:@"%@’sup world?!", @"Eyo!\n"];
    XCTAssertFalse([messenger sendMessage:message error:nullptr], @"subject should accept delegate decision");

    // At this point the mock should have recorded an invocation of messenger:canSendMessage:, retaining its arguments.
    // So this verification will succeed, and `message` is still around.
    OCMVerify([delegate messenger:subject canSendMessage:@"Eyo!\n’sup world?!"]);
}


That’s all fine and dandy: the `Messenger` has an explicitly weak reference to our mock, and the mock (via the recorded invocation) has an implicitly strong reference to the subject. No cycle here, everything is cleaned up nicely when the test finishes.

Let’s modify the `Messenger` slightly: it’ll send its message over the network, and for that it has a some APIClient. Because it needs the API client to function properly, that property is strong.
Code: Select all
NS_ASSUME_NONNULL_BEGIN

@protocol APIClient <NSObject>

typedef void (^APIClientCompletion)(BOOL success);
- (void)sendMessage:(NSString *)message withCompletion:(APIClientCompletion)completion;

@end

@protocol MessengerDelegate;

//////////////////////
@interface Messenger : NSObject

@property (readonly, nonatomic) id<APIClient> client;
@property (weak, nonatomic) id<MessengerDelegate> delegate;

- (void)sendMessage:(NSString *)message withSuccessHandler:(void (^)(void))successHandler failureHandler:(void (^)(Messenger *messenger, NSError *))failureHandler;

- (instancetype)initWithAPIClient:(id<APIClient>)client NS_DESIGNATED_INITIALIZER;
- (instancetype)init UNAVAILABLE_ATTRIBUTE;
+ (instancetype)new UNAVAILABLE_ATTRIBUTE;

@end

//////////////////////
// MessengerDelegate protocol as before

NS_ASSUME_NONNULL_END


In our test, we’d now want to stub the API client, which might look like this:

Code: Select all
// Again assuming we’re using Objective-C++ and ARC
#define ProtocolMock(protocolName) (id<protocolName>)OCMProtocolMock(@protocol(protocolName))

- (void)testThatMessengerSignalsErrorsToCallers {
    auto const client = ProtocolMock(APIClient);
    auto const subject = [[Messenger alloc] initWithAPIClient:client];

    // Assuming there’s no delegate denying the send of a message the subject will do just that via the client — which shall always fail
    OCMStub([client sendMessage:[OCMArg any] withCompletion:[OCMArg any]]).andDo(^(NSInvocation *call){
        __unsafe_unretained APIClientCompletion completion;
        [call getArgument:&completion atIndex:3];
        dispatch_async(dispatch_get_main_queue(), ^{
            completion(NO);
        });
    });

    auto const messageToSend = @"Hello network! Can you read me";
    auto const didCallFailureHandler = [self expectationWithDescription:@"Failing send should yield failure"];
    [subject sendMessage:messageToSend withSuccessHandler:^{
        XCTFail(@"Failure should never invoke success handler");
    } failureHandler:^(Messenger *sender, NSError *error){
        XCTAssertNotNil(error, @"API contract breached: %@ should have provided an error", sender);
        [didCallFailureHandler fulfill];
    }];
    [self waitForExpectationsWithTimeout:1 handler:nil];

    // At this point, the client will — again — have recorded an invocation that retains all its arguments.
    // Depending on the concrete implementation of `-[Messenger sendMessage:withSuccessHandler:failureHandler:]`, we may now have a strong reference cycle…
    OCMVerify([client sendMessage:messageToSend withCompletion:[OCMArg any]]);
    // … that will persist here, because I did not see a code path that clears out the invocations, stubs, and expectations on OCMockObject.
    // (Except of its dealloc, but that will not be called when there’s a cycle.)
}


Let’s say the messenger has been implemented so that it stays alive at least until its request goes through. The simplest possible way to achieve that is
Code: Select all
- (void)sendMessage:(NSString *)message withSuccessHandler:(void (^)(void))successHandler failureHandler:(void (^)(Messenger *, NSError *))failureHandler {
    // prevent "-Wrepeated-use-of-weak"
    auto const delegate = self.delegate;
    if (delegate != nil
        && ! [delegate messenger:self canSendMessage:message]) {
        failureHandler(self, /* assume an error goes here */);
    } else {
        [self.client sendMessage:message withCompletionHandler:^(BOOL success) {
            if (success)
                successHandler();
            else
                failureHandler(self, /* assume “server mad a booboo” error here */);
        }];
    }
}


This, however, is also one of the many possible implementations that’ll lead to a strong reference cycle in the test — although it is entirely correct on its own:

Our class under test captures itself in the block that it passes to the client — this ensures that it’ll always invoke the failure or success handler, and that it’s alive until the client finishes.
It also must have a strong reference to the API client — which in the test is the mock — for consistency. The mock will record the call, and strongly capture that block as part of the invocation, thus creating a strong reference cycle.
The problem with this is, that I didn’t see a way to break that cycle: OCMockObject appears to keep recorded invocations alive until after verification. The only code path where I saw them purged is on dealloc — which will never be called, because of the cycle.

Now, in this case there are ways out, but they are neither obvious nor pretty.
There are other interactions that are less simple to solve it for, where the easiest, most obvious way was just telling a mock “good job, now let go!”. I see the potential of harm to existing tests out there that would arise from changing the behavior of `OCMVerify()` to purge the invocation upon match.
But what about `OCMVerifyAll()` or `stopMocking`? Shouldn’t they purge everything they’ve stubbed/expected/recorded?
danyowdee
 

Re: Retention of arguments

Postby erik » 13 Oct 2015, 14:51

You are completely right about the possible retain cycles. The code of OCMock has grown over the years and some of the assumptions made early in its life, for example that a mock would always be released at the end of the test, don't hold true any more.

Solving this issue isn't straight-forward. In fact, I don't think it's possible to come up with an automatic solution. The problem is not limited to expectations. In order to make verify-after-running possible OCMock has to keep a record of every method that has been called. These invocations can be verified with different matchers and it's more or less impossible to determine when a given invocation has been verified. Even worse, though, it's unlikely that a test case will even want to verify every invocation made. So, there's likely going to be a list of invocations left over, and if one of them creates a retain cycle as you describe it's game over.

It should be possible to clear the expectations in the implementation of stopMocking as you suggest. (By the way, looking at how the current implementation ended up, it feels a bit weird that dealloc in the two subclasses for class and partial mocks calls stopMocking but the base class doesn't.) If stop mocking cleared the expectations and invocations at least it would be possible to break retain cycles manually.

As ever, making such a change is likely to have subtle effects on a lot of tests out there. In general, they should be positive, and could help with other issues, eg. people wondering why class-level stubs keep lingering. I'd like to analyse this a bit more, though.

If at all possible, could you provide a minimal example as a failing test and open an issue on Github?

thank you
erik
erik
 
Posts: 90
Joined: 10 Oct 2009, 15:22
Location: Hamburg, Germany


Return to OCMock



cron