views:

457

answers:

2

In NetBeans, there's a new hint that says: Thread.sleep called in loop.

Question 1: How/when can it be a problem to sleep in a loop?

Question 2: If it's a problem, what should I do instead?

UPDATE: Question 3: Here's some code. Tell me in this case if I should be using something else instead of Thread.Sleep in a loop. In short, this is used by a server which listen for client TCP connections. The sleep is used here in case the max number of sessions with clients is reached. In this situation, I want the application to wait until a free session get available.

public class SessionManager {
    private static final int DEFAULT_PORT = 7500;
    private static final int SLEEP_TIME = 200;
    private final DatabaseManager database = new DatabaseManager();
    private final ServerSocket serverSocket = new ServerSocket(DEFAULT_PORT);

public SessionManager() throws IOException, SQLException
{
}

public void listen()
{
while (true)
    if (Session.getSessionCount() < Session.getMaxSessionCount())
        try
        {
             new Thread(new Session(database, serverSocket.accept())).start();
        }
        catch (IOException ex) { ex.printStackTrace(); }
    else
        try
        {
            Thread.sleep(SLEEP_TIME);
        }
        catch (InterruptedException ex) { ex.printStackTrace(); }
}

public static void main(String[] args) throws IOException, SQLException
{
new SessionManager().listen();
}
}
+2  A: 

How/when can it be a problem to sleep in a loop?
People sometimes employ it in place of proper synchronization methods (like wait/notify).

If it's a problem, what should I do instead?
Depends on what you're doing. Although it's dificult for me to imagine situation where doing this is the best approach, I guess that's possible too.

You can check Sun's concurrency tutorial on this subject.

Nikita Rybak
People also sometimes use it in read loops in conjunction with testing available(), when they should just be blocking in the read instead of wasting time and space.
EJP
One usage is a directory cleaner, that deletes files older than a certain period. After the first run, we know which is the oldest file, and exactly how long until it becomes "too old", so we can sleep for that period.
Jesse
+2  A: 

Calling sleep in a loop typically leads to poor performance. For example:

while (true) {
    if (stream.available() > 0) {
       // read input
    }
    sleep(MILLISECONDS);
}

If MILLISECONDS is too large, then this code will take a long time to realize that input is available.

If MILLISECONDS is too small, then this code will waste a lot of system resources check for input that hasn't arrived yet.

Other uses of sleep in a loop are typically questionable as well. There's usually a better way.

If it's a problem, what should I do instead?

Post the code and maybe we can give you a sensible answer.

EDIT

IMO, a better way to solve the problem is to use a ThreadPoolExecutor.

Something like this:

public void listen() {
    BlockingQueue queue = new SynchronousQueue();
    ThreadPoolExecutor executor = new ThreadPoolExecutor(
            1, Session.getMaxSessionCount(), 100, TimeUnit.SECONDS, queue);
    while (true) {
        try {
            queue.submit(new Session(database, serverSocket.accept()));
        } catch (IOException ex) { 
            ex.printStackTrace();
        }
    }
}

This configures the executor to match the way your code currently works. There are a number of other ways you could do it; see the javadoc link above.

Stephen C