royneary / mod_push

Other
68 stars 19 forks source link

Push notifications not sent when stanzas remain unacknowleged #3

Open basak opened 8 years ago

basak commented 8 years ago

To track my request in https://github.com/royneary/mod_push/issues/1#issuecomment-120193763

In addition to push notifications being sent after the TCP connection for a session is broken, can they please also be sent when stanzas remain unacknowleged for some period of time (eg. 30 seconds)? Then we won't have to rely on the client specifically breaking the connection in order to receive push notifications. This matters for Ubuntu where apps are suspended without killing the TCP connections, but also for phones moving out of service areas where TCP would take considerably longer to time out. Relying instead on things like XMPP pings or TCP keepalives would consume battery life unnecessarily when the connection is inactive. XMPP session resume already requires the stanza acknowledgements anyway so there should be no additional cost to this.

royneary commented 8 years ago

Unfortunatly there's no nice way to implement this. In order to determine push events mod_push monitors three kinds of events:

  1. a client enters the xep-0198 "pending" state
  2. a client resumes its session
  3. stream management stores a stanza for later delivery

Event 3 is a push event after Event 1 occurred as long as Event 2 doesn't occur. The events are implemented as ejabberd hooks I added to ejabberd's c2s state machine. They have to be run for all clients that have resumption enabled and respectively for all incoming stanzas for those clients. Now to implement the timeout you proposed I would have to add another hook that is run on each incoming ACK and mod_push would have to monitor the 'h' values. This would add some runtime overhead with only one platform profiting but it would be possible. But how can we decide whether a user returned and does not want to receive push notifications anymore? We can't rely on Event 2 anymore because the session is not pending. We could stop sending push when the client ackknowledged all stanzas but I don't like the idea of diving that deep into stream mangement's internals. mod_push was designed to rely on stream management's resumption implementation so reimplementing it partially feels wrong. Maybe the timeout could be implemented by stream management itself but this would also affect non-push clients then.

Furthermore: Other XEP-0357 implementations would need to behave in the same way if we don't want to confuse users. AFAIK Lance Stout is working on an implementation which requires push clients to go offline in order to receive push notifications, so no ACKs at all.

I'd love to fully support Ubuntu phone but at the moment I don't know how. The best thing that could happen would the Ubuntu people implementing their app lifecycle API. At least it says "Good progress" but I don't know how long it already says so :-/

royneary commented 8 years ago

After talking to @weiss about this it doesn't seem too bad to me anymore. As you already said you can use xep-0199's server-to-client ping to let a client time out (mod_ping's timeout_action must be set to kill). It shouldn't drain the battery as the client only needs (and is able) to answer pings during the short periods when the app is active. When mod_ping's ping_interval is over the client enters the zombie state and thus will receive push notifications.

Also: In the future there might be an XEP-0198 ACK-based timeout implementation in ejabberd as all mobile clients with bad connectivity profit from short timeout intervals. But that implementation will not be part of mod_push.

basak commented 8 years ago

It shouldn't drain the battery as the client only needs (and is able) to answer pings during the short periods when the app is active.

Good point - I didn't think of that.

The best thing that could happen would the Ubuntu people implementing their app lifecycle API.

I have found that I can pick up on the onActiveChanged signal from Qt.Application as described in this AskUbuntu answer. This seems to work - I am terminating the TCP connection without closing the session when the signal arrives. However, it also activates when the user swipes from an edge (eg. top edge to look at the date or something) which isn't great. But it does mean that the app should be usable. It does still seem suboptimal though since you still have the TCP/TLS resume overhead every time a user switches between apps. It also means that if a user sends a message from the app and then immediately switches away before the app has had time to resume the session and send the message, then the message won't go out. Keeping the TCP connection would be better and XEP-0198 does make it possible.

Now I need to focus on getting a minimal app working with what we have. Right now I have the proof of concept for mod_push (thank you again for all your work on a server implementation here!) but it can't yet send messages or see the roster etc. It will be a couple of weeks before I'll have time to look at it again.

Other XEP-0357 implementations would need to behave in the same way if we don't want to confuse users.

I agree with you that different implementations should behave in the same way here. So it seems to me that the XEP is incomplete on this point: we should figure out what behaviour (ie. when exactly to send push messages) works for all platforms and then that behaviour should be defined as part of the specification, IMHO.

And, IMHO, the most sensible implementation would be to define that a push notification is sent after timeout on a stanza ack from XEP-0198, with a default of (say) 15 seconds, configurable by the client (not on the server as different client platforms will probably have different needs) and a requirement for duplicate suppression on the client end. That would seem to me to provide the best experience for users across all platforms and makes most sense from a protocol point of view (as opposed to an implementation point of view). I appreciate that this might be awkward to do in ejabberd though, and for now requiring the TCP connection to be terminated will do for an initial implementation for me.

royneary commented 8 years ago

Good to hear you found a way to close the connection.

And, IMHO, the most sensible implementation would be to define that a push notification is sent after timeout on a stanza ack from XEP-0198, with a default of (say) 15 seconds, configurable by the client (not on the server as different client platforms will probably have different needs)

I agree. I think it's possible to implement in ejabberd's stream management code. Someone just has to do it ;-) Do you have an idea how to make the timeout user-configurable?

I just implemented a feature which notifies previous push clients after a server restart. Ejabberd throws away all streams when it shuts down so clients have to reconnect. After a restart mod_push sends a push notification to all previously clients - if they were pending. I also added documentation about it to the readme. Do you think this approach can work on Ubuntu touch?

basak commented 8 years ago

I just implemented a feature which notifies previous push clients after a server restart.

That's a great idea. Thinking about it, this should also be part of the specification and mandatory for all implementations. It is inevitable that servers may eventually have to discard sessions. If we're having push notifications as a way to inform clients that sessions can be resumed, then we should also have push notifications inform clients when sessions have been terminated so they can be restarted. Otherwise it's impossible to provide a UX where users get seamless session restarts, and users will mysteriously just stop receiving messages.

Do you think this approach can work on Ubuntu touch?

I think so. Ubuntu touch won't permit apps to have network access on an incoming notification unless and until the user actually positively interacts with the notification (by going back to the app). This is by design to prevent malicious apps from doing anything nefarious triggered solely server-side without the user at least knowing that something is happening. So I cannot reconnect seamlessly, but I can inform the user through the notification that the connection has dropped and resuming when the user restarts the app.

You say in the readme that:

Currently this notification can not be distinguished from a "normal" notification (e.g. one sent in the event of an incoming message)

If you can put something in the JSON blob to differentiate the two, that would be useful. Then I could display a different sort of notification and icon, and link directly to a reconnect screen in the app. But I am nowhere near the point where I can implement this so there is no hurry.

basak commented 8 years ago

Do you have an idea how to make the timeout user-configurable?

I think this should be added to the relevant XEP. Adding it to XEP-0357 makes sense to me, along with the definition of exactly what circumstances a push notification should be sent. Add it as an attribute to the <enable xmlns='urn:xmpp:push:0'/> iq set command, perhaps with a server-side default setting and a recommendation for that default setting in the spec.