Job queue with threading

Question

On many articles and blogs, I have read that exceptions should not decide flow of your code.

I have wrote the following code using one thread:

public void run() {
        while (true) {
            if (jobQueue.size() != 0) {
                Job job =
                    (Job) jobQueue.pop();
                try {
                    JobProc jobProc = new JobProc(job);
                    Thread threadJP = new Thread(jobProc);
                    threadJP.start();
                } catch (Exception e) {
                    try {
                        jobQueue.push(job);
                    } catch (StorageFailed | InternalError e1) {
                        try {
                            writeJobToFile(job);
                        } catch (ExecFailed e2) {
                            logger.logFatal("failed to push back job"+ job.getId(), e2);
                        }
                    }
                }
                } else {
                    sleep(5000);
                }
        }
    }

In the first catch exception I can push job back to queue. In the second catch, if it fails to push it back to queue, I am writing that job to file. In third catch, I logging it as FATAL error event.

Am I doing it wrong or is there any better way of doing this?

Answer 1

Exceptions

Exceptions, by their nature, alter the control flow of the program. That's what they're there for. It's just that they should not be used for normal control flow – only in error cases. In your case, a job failing to execute is most likely an error case so I wouldn't object to the usage of exceptions here.

Question yourself, however, what kind of errors you are catching and how you handle them.

It seems that the first (outermost) try / catch block is intended to catch any exception that might be thrown by the job. This looks alright to me. We don't know the job and it might fail for any number of reasons. That should not affect our processing of further jobs. catching Exception also seems appropriate in this case, even though it is normally strongly discouraged. You could even consider catching Throwable but I like Exception better, thinking that if something that is not an Exception gets thrown, the program should probably die immediately.

Well, except that it doesn't.

You are not catching exceptions thrown by the job. Those will be thrown on its own thread and not seen by you. Instead, that thread's uncaught exception handler will deal with them. And the default exception handler – as sad as it is – simply prints a stack trace and otherwise ignores the exception. If you want to change that, you can set a custom uncaught exception handler.

The only case in which your catch gets activated is if creation of the thread fails. And in this case, you're probably in deep trouble anyway and should let your application die, eventually after logging an error. The recovery strategy in the catch is therefore inappropriate here. But let's assume for the following discussion that you've corrected this and the error handling code is in the thread that actually executes the job.

Okay, you have caught an exception from a job that failed for whatever reason. What can you reasonably do now? Your backup strategy is to simply push the job back onto the queue. I'm not sure whether this is a good idea. If some of your jobs can fail for spurious reasons, re-trying a fixed number of times might be a viable strategy. But re-trying forever almost certainly isn't. What if every once in a while you get a job that simply fails unconditionally. Then you keep pushing those jobs in your queue which will slowly but sure enough fill up with garbage jobs that will never succeed but consume precious resources and slow down the processing of jobs that have a real chance to do something useful. If you want to give each job \$n\$ chances, you could manage \$n\$ queues and move failed jobs from queue \$i\$ to queue \$i+1\$ if \$i<n\$ or else drop them onto the floor. Alternatively, you could attach a counter to each job that gets incremented for each failed attempt to execute it.

But your code also handles the case that you cannot push a job back to the queue. I'm not sure whether this is something you should handle. If you're out of memory, let your application die, maybe logging an error. I don't know the nature of your jobs but what good does it do you to have them written to a file? Will you ever re-load them from that file? And if you do, are you sure that this will be a good thing? Imagine that something awkward happens and your application gets flooded with nonsense jobs. Hopefully, it will eventually crash. But now the user re-starts it, hoping that it will behave sane again – only to find that it keeps loading the nonsense jobs from its file log and crashes again.

As a last resort, you log a fatal error. This is not an entirely bad idea. However, if all the things failed that lead up to this situation, I'd rather kill the application and not pretend everything will be fine. You can still log the error but after that, re-throw the exception and die.

I know that unit-testing error handling code can be challenging but I encourage you to do it nevertheless. Feed an intentionally poisoned job to your queue. Make sure that the exception gets caught where you think it should. If you give jobs a second chance, construct a job that fails the first but not the second time. Then construct one that always fails. Construct one that cannot be serialized. If you truly cannot construct a scenario that will execute an error handler, this might be an indicator that the handler is redundant and should be removed from the code.

As a general guideline, don't try to keep your application running no matter what. If things really get bad, giving up is the best thing you can do: „Dead Programs Tell No Lies“.

Readability

The code is not exactly readable. It already got a lot better after you've edited your question to use shorter names instead of the ones your application seems to use. Descriptive names are good but you can also overdo it. If your names simply are that long, I prefer to use auxiliary variables to keep if conditions and function calls reasonable.

What hurts readability more severely, though, is the triple nesting of try-catch blocks. I suggest you refactor this like so.

boolean handleJob(final Job job) {
    try {
        return firstStrategy(job);
    } catch (final Exception e) {
        // Maybe report the error
    }
    try {
        return secondStrategy(job);
    } catch (final Exception e) {
        // Maybe report the error
    }
    try {
        return thirdStrategy(job);
    } catch (final ThridException e) {
        // Definitely report the error and give up
        return false;
    }
}

Refactoring deeply nested control structures into separate functions and using early returns is a generally useful pattern.

Concurrency

Your code spawns a new thread for each job and immediately detaches from it. This is a bad idea. Creating threads is costly and if you create significantly more than your hardware can execute in parallel, your performance will suffer. Instead of using one thread per job, use a thread pool with a fixed number of threads. This also means that you no longer have to use the threads uncaught exception handler for dealing with exceptions thrown by your jobs but can catch them much more naturally.

I assume that your code runs on Android which already has built-in support for background jobs so you don't even have to roll the code yourself. Just check it out.

The other thing that strikes me is that you simply sleep if your queue is empty. This is not a good strategy. If you set sleepTime¹ too low, you'll waste a lot of CPU cycles (and battery) for doing nothing. If you set it too high, your application becomes unresponsive. Instead of sleeping and polling, use proper synchronization techniques. That is, wait() and notify().

---

_{¹ The question has since been edited to use a hard-coded constant instead of the variable sleepTime.}