Unsubscribe from topic during publish() callback #21

Closed
gitgrimbo opened this Issue Sep 1, 2011 · 3 comments

Projects

None yet

3 participants

@gitgrimbo

Hi,

If I try and unsubscribe my callback during the callback itself I get
the following error (in Firefox 6 - but I think browser is
irrelevant):

subscription is undefined
[Break On This Error] ret =
subscription.callback.apply( subscription.context, args );

amplify.js (line 38)

which is the 3rd line in this snippet:

for ( length = subscriptions[ topic ].length; i < length; i++ ) {
subscription = subscriptions[ topic ][ i ];
>>>> ret = subscription.callback.apply( subscription.context, args );

if ( ret === false ) {
break;
}
}

I think this is happening because my call to unsubscribe is removing a
callback from subscriptions[ topic ], but the loop does not expect
this. Sample code as follows:

function oneTimeOnlyCallback(data) {
    amplify.unsubscribe(topic, oneTimeOnlyCallback);
    doSomething(data);

}

amplify.subscribe(topic, oneTimeOnlyCallback);

My question is, is there any way to have a callback unsubscribe itself
from a topic during it receiving the notification from publish?
Without it breaking this loop.

I want to do this as I only want to be notified of a particular topic
once, and don't want the subscription for the topic left registered
after this.

Cheers.

Paul.

@gitgrimbo

Here is the patch I made to my local copy (grimbo == me), though I'm sure someone could implement it better!

publish():

    publish: function( topic ) {
        var args = slice.call( arguments, 1 ),
            subscription,
            length,
            i = 0,
            ret;

        if ( !subscriptions[ topic ] ) {
            return true;
        }

        for ( length = subscriptions[ topic ].length; i < length; i++ ) {
            subscription = subscriptions[ topic ][ i ];

            // grimbo - added null check
            if (null !== subscription) {
                ret = subscription.callback.apply( subscription.context, args );
            }

            if ( ret === false ) {
                break;
            }
        }

        // grimbo - cull nulls in case a subscription unsubscribed itself (or
        // another subscription) for this topic
        removeNullSubscriptions(topic);

        return ret !== false;
    },

removeNullSubscriptions():

// grimbo - New function to cull null subscriptions
function removeNullSubscriptions(topic) {
    if ( !subscriptions[ topic ] ) {
        return;
    }
    var i = 0,
        subscription;
    while (i < subscriptions[ topic ].length) {
        subscription = subscriptions[ topic ][ i ];
        if (null === subscription) {
            subscriptions[ topic ].splice( i, 1 );
            continue;
        }
        i++;
    }
}

unsubscribe():

    unsubscribe: function( topic, callback ) {
        if ( !subscriptions[ topic ] ) {
            return;
        }

        var length = subscriptions[ topic ].length,
            i = 0,
            // grimbo
            subscription;

        for ( ; i < length; i++ ) {
            // grimbo - test for null
            subscription = subscriptions[ topic ][ i ];
            if ( null !== subscription && subscription.callback === callback ) {
                //subscriptions[ topic ].splice( i, 1 );
                // grimbo - we can't remove because it upsets the publish loop,
                // so just set to null in case unsubscribe is called from within publish (via a subscription)
                subscriptions[ topic ][ i ] = null;
                break;
            }
        }
    }
@scottgonzalez scottgonzalez added a commit that closed this issue Oct 6, 2011
@scottgonzalez scottgonzalez Core: Handle race conditions between publish and unsubscribe. Fixes #21
… - Unsubscribe from topic during publish() callback.
80676fd
@warmrobot

Why don't you give an option to unsubscribe all callbacks of the topic (like in jquery core )? amplify.unsubscribe("items.loaded") for example.

@scottgonzalez
Contributor

That's completely unrelated to this issue.

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