thread design
I have an application that makes a severals different request call to the server inorder to initialize the application. Fortunately, these requests are not dependent upon one another; therefore, i decided it's best to do multithreading (a thread for each request).
What i'm running into is a coupling issue. I'm trying to make the application less coupling as possible. here is what i have done so far
publicclass ServletComm{
privateint totalCompleted = 0;
publicsynchronized Map processRequest(List<Request> requests){
int totalRequests = requests.size();
totalCompleted = 0;
for (Request req: requests){
(new Thread(req)).start();// don't use join, since we want all thread to start (each thread job time is different)
}
// wait until all thtreads have completed their job.
while (totalRequests != totalCompleted){
try{ Thread.wait();}
catch(InterruptedException e){}
}
totalCompleted = 0;// reset to reuse this object.
Map map =new HashMap();
for (Request req: requests){
map.put(req.getName(), req.getResponse());
}
return map;// map contains of all the responses
}
// this method is called by the Request object when the request
// completed it's task. this class will check the totalComplete to
// determine if all Request objects have completed their job.
publicsynchronizedvoid incrementTotalCompleted(){
totalCompleted++;
notifyAll();
}
}
publicclass Requestimplements Runnable{
private String name;
private Response response;
private ServletComm comm;
public Request(ServletComm comm, String name){
this.comm = comm;
this.name = name;
}
publicvoid run(){
// call the server for the response
response = processResponse(..);
comm.incrementTotalComplete();// notify the ServletComm that this thread has completed it's job
}
public Response processResponse(Object resp){
Response response =new Response();
// process the servlet response
return response;
}
pubilc String getName(){return name;}
public Response getResponse(){return response;}
}
publicclass Response{
public Response(){}
}
as you can see..the Request clas is coupled with the ServletComm..The Request object requires the ServletComm reference to update it's request status (complete).
How is this design? What would be a better design?
ps. although the request does not depends on one another..the application wil have to wait until all request are finish before it can move on. To process each request sequentially, the timeit takes to comlete the job is almost four times than using multithread. The bottlneck is with certain request..so i though to get some request done while some is still waiting for the response.
I would make a CountDownLatch with a count equal to the number of requests that must be performed. The current thread would spawn these requests, passing a common instance of CountDownLatch to each, and then await() on the CountDownLatch. The requests would then perform their job, invoke countDown() and die. Once they're all done the count will be 0 and the original thread will continue execution.
Are there any problems with the application and its performance? How does it run? Are there any problems with the dependency? Alternative approach would be to have data object keep track of request and report completion to ServletComm when all requests are complete.
> Are there any problems with the application and its performance?
some of the request takes a long time to complete, but it's out of my
control..since the information fetched is from a site not of our own.
making each request makes the application faster.
> How does it run?
It's kinda like google map, with overlay graphical information (from user
input). the map and overlay informations are from third party sites.
these requests to these sites can be time consumming.
> Are there any problems with the dependency?
the application need all request to finish before the application can be
initialize. Although the request is independent of one another..the
application need to combine the responses and use all of them to
generate new information.This is why the ServletComm needs to
know when all Request are done..and return all responses.
> Alternative approach would be to have data object keep track of
> request and report completion to ServletComm when all requests are
> complete.
I'm trying to seperate the request (communication) from the data.
Otherwise, the application would be more complex to manage. the
data (model) is being used by the view and the controller.
===============================================
> I would make a CountDownLatch with a count equal to the number of
> requests that must be performed. The current thread would spawn
> these requests, passing a common instance of CountDownLatch to
> each, and then await() on the CountDownLatch. The requests would
> then perform their job, invoke countDown() and die. Once they're all
> done the count will be 0 and the original thread will continue
> execution.
thanx kablair, I think i like this design.
public class CountDownLatch{
private int count = 0;
public CountDownLatch(int count){
this.count = count;
}
public synchronized void countDown(){
count--;
notifyAll();
}
public synchronized void await(){
while (count < 1){
try{ Thtread.wait(); }
catch(InterruptedException e){}
}
}
}
public class ServletComm{
public Map processRequest(List<Request> requests){
CountDownLatch countDownLatch = new CountDownLatch(requests.size());
for (Request req: requests){
(new Thread(req, countDownLatch)).start();
}
countdownLatch.await();
// process the servlet response code
return responses;
}
}
public class Request extends Runnable{
private CountDownLatch latch = null;
pubilc Request(CountDownLatch latch){
this.latch = latch;
}
public void run(){
try{
// perform task
latch.countDown();
}
catch (Exception e){
// TODO: figure out how to handle error occurs (like server's down);
}
finally{
latch.countDown();// make sure we count down..otherwise, ServletComm may wait forever
}
}
}
oops
try{
// perform task
latch.countDown();
}
suppose to be
try{
// perform task
// latch.countDown is only performed in the finally block
}
1. I was actually referring to java.util.concurrent.CountDownLatch which is part of Java 5. If you're not developing on 1.5 then I would roll my own version like you're doing.However I don't think your implementation will work. This is what I would do:
public final class CountDownLatch {
private int count;
// Use a private member so no external force can interfere
// with our synchronization, except with reflection of course.
private final Object lock;
public CountDownLatch(final int count) {
super();
if (count < 0)
throw new IllegalArgumentException("count must be >= 0");
lock = new Object();
this.count = count;
public void countDown() {
synchronized (lock) {
// Only notify when the count reaches 0.
if (--count == 0)
lock.notifyAll();
}
}
}
public void await() {
synchronized (lock) {
// Only wait if/while the count is greater than 0.
while (count > 0) {
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
}
}
I've never actually looked at Doug Lea's CountDownLatch (the one that's part of Java 5) source but it's probably fairly similar, except I'll bet it's got better performance and uses some of the concurrency classes he authored.
The rest of your code has some serious errors but I'm assuming those are just typos due to haste and that you get the general idea. The benefit I see to a CountDownLatch is your Request is no longer dependent on ServletComm which avoids the bad smell of a cyclical dependency.
Oops, I made some typos in mine too. I was just thinking you may want to consider calling it something other than CountDownLatch to avoid confusion with java.util.concurrent.CountDownLatch. Then again, you could always just have it wrap java.util.concurrent.CountDownLatch if/when you move to Java 5 so long as your implementation adheres to the same contract so that it's use is the same.
> > Are there any problems with the application and its
> performance?
>
> some of the request takes a long time to complete,
> but it's out of my
> control..since the information fetched is from a site
> not of our own.
> making each request makes the application faster.
So, I guess there are no problems. It is hard to tell from your response. If it's out of your control then there is nothing you can do about it.
> > How does it run?
> It's kinda like google map, with overlay graphical
> information (from user
> input). the map and overlay informations are from
> third party sites.
> these requests to these sites can be time
> consumming.
No. I refer to how the application runs with your new threaded design. Have you tried this new design out and measured the performance or are you just thinking about it and getting some theorectical ideas as to how to implement a threaded design?
>
> > Are there any problems with the dependency?
> the application need all request to finish before the
> application can be
> initialize. Although the request is independent of
> one another..the
> application need to combine the responses and use all
> of them to
> generate new information.This is why the
> ServletComm needs to
> know when all Request are done..and return all
> responses.
This was clear from you first response. What I asked are their any problems associated with the dependency of Request objects needing a reference to ServletComm. If there are no problems and the perfomance is fine, then there is nothing wrong with the Request objects having a reference to ServletComm.
>
> > Alternative approach would be to have data object
> keep track of
> > request and report completion to ServletComm when
> all requests are
> > complete.
>
> I'm trying to seperate the request (communication)
> from the data.
> Otherwise, the application would be more complex to
> manage. the
> data (model) is being used by the view and the
> controller.
The "CountDownLatch" is the data object that I refer to. It holds the data of the count of completed requests. This data object has nothing to do with the data model of your application. The data encapsulated in 'CountDownLatch" would be for the proper operation of ServletComm.
i'm constrained to java 1.4, so i cannot use the concurrency package.
and cannot use generic, enhance for loop..just use it here to make it easier to read. and yes..lots of typos..what i put down is in haste .. like Request is really an interface, but that will just only complicate thing.
i don't see any difference in locking on an instance variable (Object lock) that locking on the instance, since there are no method of this class that would be use without synchronization. or am i wrong here?
> i'm constrained to java 1.4, so i cannot use the
> concurrency package.
Then rolling your own is a good idea.
> and cannot use generic, enhance for loop..just use it
> here to make it easier to read. and yes..lots of
> typos..what i put down is in haste .. like Request is
> really an interface, but that will just only
> complicate thing.
That's what I figured. So long as you get the idea and the errors don't carry over to your actual code I'm not too concerned about correctness here.
> i don't see any difference in locking on an instance
> variable (Object lock) that locking on the instance,
> since there are no method of this class that would be
> use without synchronization. or am i wrong here?
The difference is the monitor is not exposed.Consider this:
public void doSomething() {
CountDownLatch latch = new CountDownLatch(5);
... spawn requests ...
foobar(latch);
// Deadlock.
latch.await();
}
public void foobar(Object o) {
synchronized (o) {
while (true) {
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
}
}
}
}
You are not necessarily "wrong" because no competent programmer would ever do this. However, I prefer the private member as a precaution against malicious or incompetent programmers. This issue is fairly minor.
What is important is that the notifyAll() is only invoked when it reaches 0 and that the await() method invokes wait() on the object you are synchronizing on within a loop that checks for > 0 instead of < 1.
I meant to call foobar(latch) from a new Thread.
thanx..i didn't think about that senerio.oh..and yes..the type are usually carried over into the actual project sometime (i need to take typing class or carefully look at the code for these type or error (IDE helps most of the time in caughting these types of error)) =)
