Trade off with declaring method synchronized in java?

Question

I had a problem with a thread locking up for some still unknown reason in my Android App whenever I tried to kill Thread B from Thread A (usually, sometimes it worked). I guessed that it was because some of my methods were making calls across the threads without being synchronized. I made the cancel method and a lot of methods that were essentially event handlers synchronized and made a few shared variables volatile and everything worked.

I don't know which among the 20 odd volatile/synchronized declarations I added actually solved the problem, and that got me thinking "Should I care? It works don't mess with it!"

So, my question is: Is there any trade off associated with declaring a method synchronized or a primitive volatile? Is there any reason to avoid these declarations if they are not needed?

Edit
The thread(s) in question is a Bluetooth connection that is receiving/sending streaming data, so ASyncTask and other worker thread type solutions don't work well. They are designed for performing a finite task and terminating when done. Some, like ASyncTask, also add a lot of overhead that simply kills the app. For continuously running threads like this, using a Thread is still the best way to do it.

I am using an android Service to generate and manage the threads so I am following Android design paradigms in that respect.

Answer 1

In my experience it's generally easier and often more performant to do your locking at as fine-grained a level as possible, using synchronized(object){...} for accesses to a particular object. Also, if there are several locks that you have to acquire at the same time, ALWAYS make sure that you're always acquiring them in the same order.

Static methods have their own chunk of weirdness to worry about; a synchronized instance method will only be synchronized to that instance, not to the class as a whole, and so if you need to synchronize to both in an instance method you'll need to do something like synchronized(this.class){...} as well, although if you're applying the above, really you'd just be doing synchronized() over the static fields that you're accessing in the particular method.

As another note, in general you don't want to be spawning your own threads, and instead should use the system's pre-existing mechanisms for thread management (such as ThreadPoolExecutor for ongoing work queues or AsyncTask for asynchronous UI updates). ThreadPoolExecutor tends to be more performant (and can better leverage multi-core devices), but you have to do some extra work if it's going to do things to the UI; AsyncTask, on the other hand, tends to be a bit slower and more heavyweight but it also runs its onPostExecute callback in the UI thread.

Answer 2

If it work don't fix it for now :), but for the next project you should consider using AsyncTask. Dev Guide. I don't think the performance impact isn't of real concern in Android context, but the complexity, readability and future maintainability can be a problem (cancel/kill thread, many shared variables).