Please excuse the long report - but it is probably necessary to explain such a
large patch (which I've attached as a hg 'bundle' against the current trunk)
I noticed some strangeness in how TalkMyPhone (and hence GTalkSMS) works with
its service, particularly related to the lifetime management and how other
parts of the program interact with it. To give a few practical examples:
* The UI thread is blocked while the xmpp connection is established or torn
down. In extreme cases, Android will notice this and offer to force-close the
"hung" process.
* It is possible to see exceptions raised by the various calls to
unbindService() due to the service never having been previously bound in that
context (let me know if you want a specific example to see this)
* Similarly to above, the ActivityManager log will sometimes write a
stack-trace with an error messaging saying our activity leaked a
ServiceConnection.
I decided to have a go at fixing that :) I made some significant structural
changes which I'll outline below:
* The service is implemented with a thread+queue system along the lines of
'IntentService' (but alas, IntentService itself wasn't flexible enough to use
directly). All interaction with the XmppManager now happens on a worker
thread. Nothing on the main thread blocks at all meaning the UI is noticeably
snappier.
* All communication with the worker thread is done using Intent objects. The
'instance/getInstance()' statics have gone (yay!) - everyone who used to use
that now uses Intents.
* All communication from the worker thread to the rest of the system is done
using broadcast receivers. Eg, the XmppListener class has been removed and
replaced with generic broadcasts when connection-status changes, a message is
received or presence changes. The service no longer needs to delegate to other
listeners - android manages all that for free.
* I removed the CallReceiver class and arrange to hook up telephony
notifications only when our service is running, preventing the GTalkSMS process
from starting every time a call is received just to notice we are not connected
and to do nothing.
* The above all enables the service lifetime issues to be made much cleaner.
stopService() and onDestroy() are never explicitly called - android can kill
the process when it is necessary. The service isn't 'destroyed' (although it
is 'stopped') when it is disconnected, meaning activities can bind to the
service in the "usual" way without fear the service went away just because the
connection died.
* The XmppManager state of "QUIT" has been removed as it is no longer
necessary. A new state of WAITING_TO_CONNECT has been added which indicates an
automatic connection attempt is pending (due to either lack of network
capability or a retry cycle). The service remains in the foreground while in
this state to avoid premature stopping of the service.
Another side-effect of this set of changes is that if the XmppManager does
hang, it doesn't cause the entire app to hang - just the worker thread gets
stuck. A side-effect of this though is that android will no longer offer the
"force close" dialog as the app's main thread is still working perfectly. This
is still a better result when taking the widget into consideration, where a
hang is much more noticeable.
The attached patch set is very large, but I think very worthwhile. I
understand it may take a little time to go through all the changes - please ask
me if you need any clarification. I'm also happy to remerge the patches should
other changes be made on the trunk while this is being reviewed. FWIW, I have
tested it quite thoroughly.
Original issue reported on code.google.com by skippy.hammond@gmail.com on 29 Dec 2010 at 6:45
Original issue reported on code.google.com by
skippy.hammond@gmail.com
on 29 Dec 2010 at 6:45Attachments: