Fix #640

[ricardopereira]

#640

[ricardopereira]

d6353e1 test raises a EXC_BAD_INSTRUCTION in [ARTRealtimeChannel unsubscribe] because of a _dispatch_barrier_sync_f_slow :

[tcard]

@ricardopereira This is expected. You can't call unsubscribe from an internalEventEmitter, since a internalEventEmitter works exclusively within the internal queue, already holding the lock.

I'll comment on the issue what I think the problem is.

[ricardopereira]

(3e1bd48) Another try to reproduce the issue but no success.

[ricardopereira]

@tcard I think we just need to change the ARTInternalEventEmitter to ARTPublicEventEmitter.

https://github.com/ably/ably-ios/blob/2027ee3690f5fa2af3c1f629d8471934d6e79348/Source/ARTRealtimeChannel.m#L418-L420

https://github.com/ably/ably-ios/blob/2027ee3690f5fa2af3c1f629d8471934d6e79348/Source/ARTRealtimeChannel.m#L66

[tcard]

@ricardopereira No, that's for dispatching to the internal queue when the user is in the user (main) queue. This is the other way around, from the internal queue we need to dispatch to the user queue. And this needs to happen for internal event emitters too, at least those that potentially hold references to user objects.

[ricardopereira]

This is the other way around, from the internal queue we need to dispatch to the user queue.

@tcard But we are already doing that by encapsulating each user callback with our implementation which dispatches the callback to the user queue:

    if (cb) {
        void (^userCallback)(ARTMessage *__art_nullable m) = cb;
        cb = ^(ARTMessage *__art_nullable m) {
            ART_EXITING_ABLY_CODE(_realtime.rest);
            dispatch_async(_userQueue, ^{
                userCallback(m);
            });
        };
    }

[tcard]

@ricardopereira Right, and that's perfect, but what I didn't anticipate is that there's another way of executing user code from our internal queue: by causing some of their object's reference counts to drop to zero, hence automatically calling their dealloc method. Which is exactly what happened at #640.

When off is called, we drop the reference to a callback, and that callback may be holding references to other objects, and those objects may have a dealloc method. Hence, dropping references to user callbacks must be executed in the user queue, not in our internal queue.

[tcard]

This should be fixed.

[tcard]

I think we can remove this one, the other reproduces the issue more clearly from a user persoective without meddling with the internal emitter.

[tcard]

Hmm, does this really reproduce the issue? I think if you call close here, then it will remove the listener you add at init but since you still have a reference to the created object its deinit won't be called, and when you drop the reference just after, since you already removed the listener, its deinit's unsubscribe will just do nothing.

I think deinit should just call unsubscribe directly; we don't need close.

Please confirm this reproduces the issue by commenting out the dispatch_async in removeListener and making sure this test then fails, and stops failing if you put it back.

[ricardopereira]

Hmm, does this really reproduce the issue?

I think so but it always pass. I'll change the code with your points. Thanks.

[ricardopereira]

It always passes d0b8aeb

[ricardopereira]

Should we ask the client to test using the branch?

[tcard]

@ricardopereira Yes, but we also need to have a test that actually reproduces the problem. Maybe instead of self we can reference a different object, then make sure we drop the other reference to it before dropping the reference to foo?

[ricardopereira]

Success!

[tcard]

Nice :)

[ricardopereira]

Squashed.