HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Simple Background Thread Worker Class (follow-up)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplebackgroundworkerthreadfollowclass

Problem

This is a follow up of Simple, generic background thread worker class

What changed?

  • Moved from synchronized lazily evaluated static instances to true Singletons per the on-demand holder idiom https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom



  • Added a constructor to auto-start the Thread.



Why is this being resubmitted?

Some of the changes introduce a potential anti-pattern that I'd appreciate feedback on. Additionally, I believe the feedback I had originally was fair, thoughtful and valuable, but did not necessarily address some of the points I was concerned about. This was my fault since I did not provide specifics. I'll do that now.

The original review included some feedback about static versus instance method conventions which were not addressed in the rewrite - this is intentional - I'm using static members for the Handler and ThreadPoolExecutor so that they're common to each instance of the class, but can also be overridden by subclasses to provide different implementations.

What specifically should be evaluated?

-
Overall ease of use, and usefulness: would this suffice as a replacement (not a drop-in) for most of what AsyncTask is being used for? A design goal was to delegate most responsibilities to the user (to test and react if dependencies exist - e.g., did the Activity that was going to display the finished work get paused/destroyed? if so, e.g., let's stop decoding the bitmap) and provide them with a simple mechanism to do that: the quitIfCancelledOrInterrupted method, which should be tested as frequently as possible within the body of a Runnable passed to runOnUiThread, and tests if cancel has been called or if the Thread has been interrupted otherwise - either way, a new interrupt request is sent, and a true value is returned, to which the user code should reply by immediately exiting (return from the run method).

-
Is there any controversy about the specific Singleton pattern used here? I know tha

Solution

Escape of this on the constructor

When you call start the thread may receive the current object (this) and start execution immediately.
This means that your constructor may not have run fully and your object is not ready for use yet.
The only thing you can do to avoid this is to call start after the constructor.

This issue is more common on event listeners implementations.

Concurrency

You are using volatile correctly and it really suffices your currency needs.
However people tend to prefer to user AtomicBoolean and AtomicReference.

Code that you do not control

The programmer implementing performWorkInBackgroundThread may throw
an exception on is implementation there is no really a need for your
worker thread to propagate that exception to nowhere: catch it.

All the cancelling stuff

(Again) The programmer only needs to implement performWorkInBackgroundThread.
In this implementation he usually does two things:

  • He implements a heavy processing/IO task that takes some time



  • He updates the user interface



What you really want to do here is just to have a way to cancel the
heavy processing task.
There is no really a need for him to know about cancelled or interrupted status.
Thus isCancelled, isInterrupted, isCancelledOrInterrupted and quitIfCancelledOrInterrupted are all meaningless.
And cancel doesn't need any arguments at all, after all it's being explicitly called so it should cancel no matter what.

If you really want to provide a way to him implement something when the heavy work is cancelled then provide another method like onWorkCanceled.

Number of threads

The number of threads is one of those things that you have to profile to make sure to see which number is the best one. But I would say that you are using a fair approach.

There is no harm of having one thread idle lying around. If your API is being used it's because someone needs it, thus that thread will work at some point (Or so we hope at least).

At maximum you have as many threads as your parallelism allows meaning that you can take advantage of all CPUs on the device.

There is one particular scenario where the maximum threads may need to increase. If you keep doing operations that take loads of time and those operations blocks the thread the BLOCKING_DEQUE keeps increasing with stuff to do. If maximum threads would be larger a new thread could come into the scene and work while one of those threads is blocked.

See here a reference implementation of AsyncTask

Context

StackExchange Code Review Q#129973, answer score: 5

Revisions (0)

No revisions yet.