appsquickly / typhoon

Powerful dependency injection for Objective-C ✨✨ (https://PILGRIM.PH is the pure Swift successor to Typhoon!!)✨✨
https://pilgrim.ph
Apache License 2.0
2.7k stars 270 forks source link

injectMethod with one parameter doesn't work under certain circumstances on iOS 11 #573

Closed lan83 closed 6 years ago

lan83 commented 6 years ago

When calling objectEnumerator of an NSArray instance containing a single object in iOS 11, the returned NSEnumerator instance doesn't retain the array anymore. As a consequence when iterating over the injectedParameters array in createInvocationWithContext:completion: the array of parameters gets deallocated to early and the invocation never gets invoked:

TyphoonMethod+InstanceBuilder.m

- (void)createInvocationWithContext:(TyphoonInjectionContext *)context completion:(void(^)(NSInvocation *invocation))result
{
    BOOL isClassMethod = [self isClassMethodOnClass:context.classUnderConstruction];

    NSMethodSignature *signature = [self methodSignatureWithTarget:context.classUnderConstruction isClassMethod:isClassMethod];
    NSInvocation *invocation = [NSInvocation invocationWithMethodSignature:signature];
    [invocation retainArguments];
    [invocation setSelector:_selector];

//[self injectedParameters] returns a copy. We expect that the enumerator retains the copy but it doesn't on iOS 11
    [[self injectedParameters] typhoon_enumerateObjectsWithManualIteration:^(id<TyphoonParameterInjection> object, id<TyphoonIterator> iterator) {
        NSUInteger index = [object parameterIndex] + 2;
        context.destinationType = [TyphoonTypeDescriptor descriptorWithEncodedType:[signature getArgumentTypeAtIndex:index]];
        [object valueToInjectWithContext:context completion:^(id value) {

//when this block is executed asynchronously (e.g. when we have to wait for the instance on circular dependencies), 
//the copy of the array the enumerator is working with has been deallocated because the enumerator
//hasn't retained the array but we've expected the enumerator to keep the array instance alive.
//weakSelf in the nextBlock is nil and the completion block is never called.

            [invocation typhoon_setArgumentObject:value atIndex:(NSInteger)index];
            [iterator next];
        }];
    } completion:^{
        result(invocation);
    }];
}

NSArray+TyphoonManualEnumeration.m

- (void)typhoon_enumerateObjectsWithManualIteration:(TyphoonManualIterationBlock)block completion:(dispatch_block_t)completion
{
    TyphoonNextSignal *signal = [[TyphoonNextSignal alloc] init];

    NSEnumerator *objectsEnumerator = [self objectEnumerator];

    __weak __typeof (self) weakSelf = self;
    __weak __typeof (signal) weakSignal = signal;

    [signal setNextBlock:^{
//weakSelf can be nil potentially. In this case completion will never be executed.
        [weakSelf typhoon_doStepWithEnumerator:objectsEnumerator signal:weakSignal block:block completion:completion];
    }];

    [self typhoon_doStepWithEnumerator:objectsEnumerator signal:signal block:block completion:completion];
}

- (void)typhoon_doStepWithEnumerator:(NSEnumerator *)enumerator signal:(id<TyphoonIterator>)iterator block:(TyphoonManualIterationBlock)block completion:(dispatch_block_t)completion
{
    id object = [enumerator nextObject];
    if (object) {
        block(object, iterator);
    } else {
        completion();
    }
}

To fix this issue, self should be captured strongly inside the nextBlock:

    __block __typeof (self) strongSelf = self;
    __weak __typeof (signal) weakSignal = signal;

    [signal setNextBlock:^{
        BOOL enumeratorExhausted = [strongSelf typhoon_doStepWithEnumerator:objectsEnumerator signal:weakSignal block:block completion:completion];
        if(enumeratorExhausted){
            strongSelf = nil;
        }
    }];

When having methods with multiple parameters, the enumerator retains the array and everything is working as expected.

alexgarbarev commented 6 years ago

Hey @lan83, could you please assist me in this bug reproduction? I've tried, but no luck.. I did this code:

- (NSArray *)dummyArray
{
    return [@[@1, @2, @3, @4, @5, @6, @7] copy];
}

- (void)test_array_enumerator
{
    XCTestExpectation *expectation = [self expectationWithDescription:@"Waiting for manual iteration"];

    [[self dummyArray] typhoon_enumerateObjectsWithManualIteration:^(id object, id<TyphoonIterator> iterator) {
        [(NSObject *)iterator performSelector:@selector(next) withObject:nil afterDelay:0.5];
    } completion:^{
        [expectation fulfill];
    }];

    [self waitForExpectationsWithTimeout:10 handler:^(NSError * _Nullable error) {

    }];
}

But it passed with weak/strong self inside for iOS9, 10 and 11.

lan83 commented 6 years ago

Hi @alexgarbarev thanks for your reply. The problem only occurs with arrays containing a single element. I wrote a test that uses a similar setup as - (void)createInvocationWithContext:(TyphoonInjectionContext *)context completion:(void(^)(NSInvocation *invocation))result This test fails in iOS 11 while it passes on iOS 10:

@interface EnumTestTests : XCTestCase {
    NSMutableArray *_dummyArray;
}

@end

@implementation EnumTestTests

- (void)setUp {
    [super setUp];
    _dummyArray = [NSMutableArray arrayWithObjects:@"foo", nil];
}

- (void)tearDown {
    // Put teardown code here. This method is called after the invocation of each test method in the class.
    [super tearDown];
}

- (NSArray *)dummyArray
{
    return [_dummyArray copy];
}

- (void)test_array_enumerator
{
    XCTestExpectation *expectation = [self expectationWithDescription:@"Waiting for manual iteration"];
    [[self dummyArray] typhoon_enumerateObjectsWithManualIteration:^(id object, id<TyphoonIterator> iterator) {
        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
            [iterator next];
        });
    } completion:^{
        [expectation fulfill];
    }];

    [self waitForExpectationsWithTimeout:1 handler:^(NSError * _Nullable error) {

    }];
}

@end

The main difference is that objectEnumerator returns a NSSingleObjectEnumerator in iOS 11 while it returns a NSFastEnumerationEnumerator in iOS 10. iOS 11:

bildschirmfoto 2017-09-14 um 16 20 47

iOS 10:

bildschirmfoto 2017-09-14 um 16 24 49
alexgarbarev commented 6 years ago

Hey @lan83. Thanks for that detailed response. I guess the problem in different versions of iOS11. I have one of first beta releases (Version 9.0 beta (9M136h)), so I had NSFastEnumerationEnumerator on both iOS 10 and iOS 11. Anyway you did amazing work on catching this, thanks!

Proposed changes merged to master and release as 4.0.3