patternjavaMinor
Simple Background Thread Worker Class (follow-up)
Viewed 0 times
simplebackgroundworkerthreadfollowclass
Problem
This is a follow up of Simple, generic background thread worker class
What changed?
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
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
-
Is there any controversy about the specific Singleton pattern used here? I know tha
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
When you call
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
This issue is more common on event listeners implementations.
Concurrency
You are using
However people tend to prefer to user
Code that you do not control
The programmer implementing
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
In this implementation he usually does two things:
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
And
If you really want to provide a way to him implement something when the heavy work is cancelled then provide another method like
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
this on the constructorWhen 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 throwan 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
AsyncTaskContext
StackExchange Code Review Q#129973, answer score: 5
Revisions (0)
No revisions yet.