How to deal with: Call to 'Thread.sleep()' in a loop, probably busy-waiting

Question

Guys how to deal with such code and warning?

private void listenOnLogForResult() {
    String logs = "";
    int timeCounter = 1;
    while (logs.isEmpty()) {
        try {
            timeCounter++;
            Thread.sleep(2000); // Wait 2 seconds
        } catch (InterruptedException e) {
            log.error(e.getLocalizedMessage(), e);
        }
        if (timeCounter < 30) {
            logs = checkLogs()
        } else {
            logs = "Time out";
        }
    }
}

I need to pause current thread for 2 seconds to wait file to be filled, but my Intelij Rise an issue here. And also I am getting error from sonar: SonarLint: Either re-interrupt this method or rethrow the "InterruptedException".

I've tried already with many ExecutorService, but it is always run in seperate thread, and I need to pause current one.

Please help..

Crespi

May be this will help #54394542. For your exception, you are just logging it and the while loop continues. That is a potential candidate for an infinite loop I guess – Greenfinch

Answer 1

The busy-waiting warning

This is a warning coming from IntelliJ that is dubious, in the sense that what you're doing is often just straight up required. In other words, it is detecting a pattern that is overused, but whose usage cannot be reduced to 0. So, likely the right solution is to just tell IntelliJ to shut up about it here.

The problem it is looking at is not that Thread.sleep. That is not the problem. However, IntelliJ's detector of this pattern needs it to find this case, but it is not what it is complaining about, which might be a little hard to wrap your head around.

What IntelliJ is worried about, is that you're wasting cycles continually rechecking log.isEmpty() for no reason. It has a problem with the while aspect of this code, not the sleep. It would prefer to see code where you invoke some sort of logs.poll() method which will just wait until it is actively woken up by the act of new logs appearing.

If this is all running within a single java process, then you can indeed rewrite this entire system (which includes rewrites to whatever log is here, and a complete re-imagining of the checkLogs() method: Instead of going out and checking, whatever is making logs needs to wake up this code instead.

If it's not, it is likely that you need to tell IntelliJ to shut it: What you are doing is unavoidable without a complete system redesign.

The re-interrupt warning

You have some deplorable exception handling here.

Your exception handling in general

Do not write catch blocks that log something and keep moving. This is really bad error handling: The system's variables and fields are now in an unknown state (you just caught and logged some stuff: Surely that means you have no idea what conditions have occurred to cause this line of execution to happen!), and yet code will move right along. It is extremely likely that 'catch exceptions and just keep going' style code results in more exceptions down the line: Generally, code that operates on unknown state is going to crash and burn sooner rather than later.

Then, if that crash-and-burn is dealt with in the same fashion (catch it, log it, keep going), then you get another crash-and-burn. You end up with code that will, upon hitting a problem, print 186 exceptions to the log and they are all utterly irrelevant except the first one. That's bad yuyu.

You're also making it completely impossible for calling code to recover. The point of exceptions is that they need to bubble upwards endlessly: Either the exception is caught by code that actually knows how to deal with the problem (and logging it is not dealing with it!), which you are making impossible, or, the code exception should bubble up all the way to the entry-point handler which is the right place to log the error and abort the entry-point handler.

An entry-point handler is a generic module or application runner; out of the box, the code baked into java.exe itself that ends up invoking your psv main() method is the most obvious 'entry point runner', but there's more: Web frameworks will eventually invoke some code of yours that is supposed to handle a web request: That code of yours is analogous to psv main(): It is the entry-point, and the code in the web framework that invokes it, is the entry-point runner.

Entry-point runners have a good reason to catch (Throwable t), and to spend their catch block primarily logging it, though they should generally log a lot more than just the exception (a web handler should, for example, log the request details, such as which HTTP params were sent and which path request it was, maybe the headers, etc). Any other code should never do this, though.

If you have no idea what to do and don't want to think about what that exception might mean, the correct 'whatever, just compile already javac' code strategy is to add the exception type to your throws line. If that is not feasible, the right code in the catch block is:

} catch (ExceptionIDoNotWantToThinkAboutRightNow e) {
    throw new RuntimeException("Uncaught", e);
}

This will ensure that code will not just merrily continue onwards, operating on unknown state, and will ensure you get complete details in logs, and ensures that calling code can catch and deal with it if it can, and ensures that any custom logging info such as the HTTP request details get a chance to make it to the logs. Win-win-win-win.

This case in particular: What does InterruptedEx mean?

When some code running in that java process invokes yourThread.interrupt(), that is how InterruptedException can happen, and it cannot possibly happen in any other way. If the user hits CTRL+C, or goes into task manager and clicks 'end process', or if your android phone decides it is time for your app to get out as the memory is needed for something else - none of those cases can possibly result in InterruptedExceptions. Your threads just get killed midstep by java (if you want to act on shutdowns, use Runtime.getRuntime().addShutdownHook). The only way is for some code to call .interrupt(), and nothing in the core libs is going to do that. Thus, InterruptedException means whatever you think 'call .interrupt() on this thread' means. It is up to you.

The most common definition is effectively 'I ask you to stop': Just shut down the thread nicely. Generally it is bad to try to shut down threads nicely if you want to exit the entire VM (just invoke System.shutdown - you already need to deal with users hitting CTRL+C, why write shutdown code twice in different ways?) - but sometimes you just want one thread to stop. So, usually the best code to put in a catch (InterruptedException e) block is just return; and nothing else. Don't log anything: The 'interrupt' is intentional: You wrote it. Most likely that is nowhere in your code base and the InterruptedException is moot: It won't ever happen.

In your specific code, what happens if your code decides to stop the logger thread is that the logger thread will log something to the error logs, and will then shortcut its 2 second wait period to immediately check the logs, and then just keeps going. That sounds completely useless.

But, it means whatever you want it to. If you want an ability for e.g. the user to hit a 'force check the logs right now' button, then you can define that interrupting the logging thread just shortcuts the 2 seconds (but then just have an empty catch block with a comment explaining that this is how you designed it, obviously don't log it). If you ALSO want a button to 'stop the logging thread', have an AtomicBoolean that tracks 'running' state: When the 'stop log-refreshes' button is hit, set the AB to 'false' and then interrupt the thread: Then the code you pasted needs to check the AB and return; to close the thread if it is false.

Impoverish

I find it strange that you passionately decried the exception handling approach here (using words like "deplorable" and lots of bold text), then conceded in your last paragraph that "you can define that interrupting the logging thread just shortcuts the 2 seconds". I think that is exactly what OP was trying to define it as, and I also use this pattern regularly. in fact, unless you are building a complex multi-threaded application with interrupt mechanisms, that's usually what interrupt means, i.e. it means "hey SomeThread, stop sleeping and get on with your job" – Hockett

@AdamBurley That is clearly a false proposition. If OPs intent was to do that, they wouldn't send out a log message at ERROR level, obviously. Your secondary idea that interrupt usually means 'stop waiting and check immediately, but just keep doing your thing otherwise' is also just stuff you made up. It's fine to that, but it's not "the right choice" or even "the most common choice". It's a choice. As good as many others. – Impoverish

What about Thread.currentThread().interrupt(); ? I thought it's a common practice when catching InterruptedException but I see you didn't mention it. I'm curious as to why. Thanks in advance. – Indigestive

Interrupting a thread 'raises the flag'. The first thing that 'handles' an interrupt lowers it; Thread.interrupted() is considered 'handling it' (that returns true if the flag is up and lowers the flag), because the assumption is that the content of that if block handles it. throwing InterruptedEx is also considered handling it. It should be: After all, if you throw IntEx and keep the flag up, the code that handles the intex is going to get a very nasty surprise! It might do something innocuous, such as 'log something', and that will fail as the I/O to send to log file interrupts. – Impoverish

So should you raise the flag? The commonly received knowledge is completely wrong: PROBABLY NOT. The idea 'just re-raise it, its better that way' is dangerously detrimental for that very reason (it asplodes any attempt to handle the situation). There are specific cases where you SHOULD re-raise it - if you aren't so much handling it, you're just interjecting a few things and then letting the interrupted state propagate. It depends. But usually no, and if one MUST write a blanket rule, 'do not reraise it' is vastly superior to 're-raise it'. – Impoverish

The specific point I've tried to make in this answer is: Interrupts mean what you say they mean (because they don't happen unless you write .interrupt() in your code somewhere) and there are no significant established conventions. Thus, [1] If you don't use the system don't try to deal with unspecified things that cannot happen anyway, and [2] If you DO use it, decide what it means and act accordingly. For libraries who don't know what happens - be simple and consistent; your only real way to do that, is to throw InterruptedEx or some other exception and keep that flag LOWERED. – Impoverish

Answer 2

fun sleep(timeMillis: Long) {
    val currentTimeMillis = System.currentTimeMillis()
    while (true) {
        if (System.currentTimeMillis() - currentTimeMillis >= timeMillis) {
            break
        }
    }
}

and use this in your method(It's code by koltin,you should trans to java)

Christly

This is literally busy-waiting, which is even worse than Thread.sleep. And Kotlin is not Java. – Annotation