`onUnregistered` message is not always sent by `unregisterApp`

[nikclayton]

My expectation, from the docs, was that calling unregisterApp() should always result in the receiver's onUnregistered method being called.

It's not, and I think that could be a problem.

Suppose I use onUnregistered to make an API call to my remote server to disable push messages. So something like:

@AndroidEntryPoint
class UnifiedPushBroadcastReceiver : MessagingReceiver() {
    @Inject
    lateinit var api: RemoteApi

    // ...

    override fun onUnregistered(context: Context, instance: String) {
        api.unsubscribePush(instance)  // HTTP DELETE /api/v1/push/:instance
    }
}

Elsewhere in my code I allow the user to log out from their account, and as part of the log out logic I have this:

suspend fun logout(context: Context, account: Account) {
    // ...
   
    UnifiedPush.unregisterApp(context, account.unifiedPushInstance)

    // ...
}

My expectation is the call to unregisterApp() will send the broadcast message, onUnregistered will be called, and an RPC to the server to unsubscribe from push messages will be sent.

However...

unregisterApp() may return early, without sending the broadcast message. If I'm reading the code correctly this can happen if the user deletes the app they're using as the push distributor.

For example, suppose:

1. The user installs my app
2. My app tells them to install a push distributor
3. They decide to install ntfy
4. After trying my app they decide they don't want to use it any more
5. They uninstall ntfy
6. They log out of my app, then uninstall it

Because they uninstalled ntfy before logging out of my app onUnregistered is not called, and the push subscription is never deleted from the server.

[p1gp1g]

Thank you for your issue, the documentation will be updated. Related issue: https://codeberg.org/UnifiedPush/documentation/issues/4

[nikclayton]

I don't think this is just a documentation issue, it's a how-easy-is-it-to-use-the-library issue.

For example, at the moment it's not possible to write clean code that uses this library. I've had to resort to a hack that does roughly the following:

To handle logging out

fun logout(account: Account) {
    // ...
    disablePushNotifications(account)
   // ..
}

To handle unregistration caused by the user removing the app from the distributor

@AndroidEntryPoint
class UnifiedPushBroadcastReceiver : MessagingReceiver() {
    // ...

    override fun onUnregistered(context: Context, instance: String) {
        disablePushNotifications(Account.fromUnifiedPushInstance(instance))
    }
}

The function that actually does the disabling

suspend fun disablePushNotifications(account: Account) {
    // onUnregistered() calls this function to unregister. But this function
    // also calls unregisterApp(), which calls onUnregistered(), which calls
    // this function again. So we risk calling api.unsubscribePush() twice,
    // and if this function was more complex, duplicating other work too.
    //
    // To prevent this, check to see if the account's push endpoint is
    // blank. If it is this code has already run once
    if (account.unifiedPushUrl.isBlank()) return

    account.unifiedPushUrl = ""
    account.save()

    // Tell the server to unsubscribe
    launch {
        api.unsubscribePush(instance)  // HTTP DELETE /api/v1/push/:instance
    }

    // Unregister from the UP provider
    UnifiedPush.unregisterApp(context, account.unifiedPushInstance)
}

It's not obvious the code needs to do something like that, and it increases the fragility of the code.

This could be fixed by either:

1. Changing the contract for unregisterApp(), and always broadcast so onUnregistered is guaranteed to be called.
2. Introduce a new onUnregistrationFailed(context: Context, instance: String) method for the MessagingReceiver to implement. Broadcast the message that triggers this in all the places on unregisterApp() currently performs an early return. Possibly with an additional parameter that indicates why unregistration failed.

[p1gp1g]

onUnregistered() calls this function to unregister. But this function
also calls unregisterApp(), which calls onUnregistered(), which calls
this function again...

This should not happen, when you call unregisterApp, it removes the token linked to your connection, so onUnregistered is not called again. If you see the loop, then there is definitely an issue. Can you confirm you see this loop ? Would it be possible to have more logs or something to debug this ?

For example, on UP-Example when unregister is called, even without the forceRemoveDistributor, onUnregistered is never called.

[nikclayton]

I updated the comment in the code to address that (I think) and be more precise. If my understanding is correct, in the code above, the call chain if the user logs out will look like:

logout() -> disablePushNotifications() -> unregisterApp() -> ... -> onUnregistered() -> disablePushNotifications()

So disablePushNotifications() is called twice.

[p1gp1g]

Here are the 2 possible chains:

1. User logging out:

logout() -> disablePushNotifications() -> unregisterApp() -> END (the distributor remove the registration, onUnregistered is never called)

2. Distributor unregister the app:

onUnregistered() -> disablePusNotification() -> unregisterApp() -> END (The distributor have already remove it)

[nikclayton]

logout() -> disablePushNotifications() -> unregisterApp() -> END (the distributor remove the registration, onUnregistered is never called)

That connector behaviour is surprising, given the spec.

org.unifiedpush.android.distributor.UNREGISTER

The connector sends this action to unregister from push messages. The intent MUST contain 1 extra:

• token (String): the token supplied by the end user application during registration

and then

org.unifiedpush.android.connector.UNREGISTERED

The distributor MUST send this action to the registered application to inform it about unregistration.

The intent MUST have the following extra:

• token (String): the token supplied by the end user application during registration

There's nothing in the spec that allows the connector to receive UNREGISTERED and not pass it on to the application.

Is this a doc bug or a connector bug?

[p1gp1g]

What happen behind the scene is:

logout() -> disablePushNotifications() -> unregisterApp() -> [ The connector remove the token then sends UNREGISTER, the distributor receives the intent, remove the registration then sends UNREGISTERED with the same token, the connector received the intent with a token it doesn't have, and ignore the request ] = END

This is totally compliant with the specifications, however there are some liberties regarding the implementation, it has be chosen to remove the token with unregister and to ignore the UNREGISTERED following that intent.

This UNREGISTERED could be controlled just to inform if something is blocking, but that way, the distributor can't leave the application in an inconsistent state. For instance, if a distributor waits to get connected to its server to ACK the unregistration (which is not the best way to implement, I agree), then the application can't change distributor until this one is connected, or if there is an option to force remove the distrib in the application. Which is very bad UX. It is better to have a registration not totally removed on the push server (which shouldn't be used anyway)