viernes, 11 de mayo de 2012

Warning!!!! Thread.sleep called in loop

Wow serveral months since last post!

Well returning to the topic, when you write a code block like this:
 while(timeout()){  
  try{  
   Thread.sleep(1);  
  }catch(Exception e){}  
 }  
Some IDEs as Netbeans or some software quality tools popup a warning informing that this kind of code usually is a design flaw or a bad practice, in other words a code smell. If you search in google you will get quick answers like "if you put a Thread.sleep inside a loop you are writting inneficient code, you should use notify and wait" but at the end of the day I have never found a clear example of how this code conversion from a "while" loop to a monitored code should be done. So.... here's the example, lets think an hipotetical problem where one thread is pooling a message bus. If you send a message trough the message bus you must wait an amount of time for another message that is a response for the sent message. The amateur way of implementing this would look like:
 Message lastMessage = null;  
 void sendMessage(Message messageToSend){  
   messageToSend.send();  
   while(!timeout()){  
    if(lastMessage != null){  
     reponseReceived(lastMessage);  
     lastMessage = null;  
    }  
    try{Thread.sleep(sleepTime);}catch(Exception e){}  
   }   
 }  
 //The message bus listener code, this is not that relevant  
 //We can just assume that this method is called from other thread.  
 void messageFoundInBus(Message responseMessage){  
  lastMessage = responseMessage;  
 }  
This code works but have two main problems. First, this is not efficient because sleepTime will put the Thread to sleep for a fixed amount of time. E.g. If you have 10 seconds in this constant the worst scenario is receiving the Message just after start sleeping making the Thread wait 9.x seconds to be aware of the response even if the message arrived long time ago. You could say, just use a smaller value but that could cause a different problem because if the sleep time is to short or you even decide to not put the Thread.sleep you could face starvation. The second problem is that while you are sleeping you could get a second message and override your response. There's no locking code. You could just include some synchronized sentences but you could forget it just because you are not forced to synchronize anything. If you use notify and wait you will be forced to synchronize this code avoiding this kind of problems from the beggining. Here's a second aproach using notify and synchronize:
 Message lastMessage = null;  
 final Object responseMonitor = new Object();  
 void sendMessage(Message messageToSend){  
   try{  
    messageToSend.send();    
    if(lastMessage != null){  
     synchronized (responseMonitor) {  
       responseMonitor.wait(timeout);
     }     
    }  
    if(lastMessage != null){  
     reponseReceived(lastMessage);  
     lastMessage = null;  
    }  
   }catch(Exception e){}  
 }  
 //The message bus listener code, this is not that relevant  
 //We can just assume that this method is called from other thread.  
 Message lastMessage = null;  
 void messageFoundInBus(Message responseMessage){  
  synchronized (responseMonitor) {  
   lastMessage = responseMessage;  
   responseMonitor.notify()  
  }  
 }  
The solution that I present is not an absolute rule, there are a lot of reasons not related to wait for a resource that could bring you to need a loop with a sleep inside, but almost every time you should be able to find a better way in order to avoid this kind of code.

1 comentario: