2

I just wrote this code in my runnable's run() method: 

try {
    dbConnection = MyApp.datasource.getConnection();
} catch (SQLException e) {
    logger.log(Level.SEVERE, "Could not obtain a DB connection! Re-enqueuing this task. Message: " + e.getMessage(), e);
    MyApp.executor.execute(this);
    return;
}

As you can see, if the task can't obtain a DB connection, it should re-enqueue itself, into the same queue it was in before it ran.

I'm thinking this is probably safe, but it feels funny, and I just want to make sure there aren't any gotchyas that I'm missing.

Thanks!

Avi Flax
  • 50,872
  • 9
  • 47
  • 64
  • 2
    Maybe you want to add some `Thread.sleep()` before re-enqueuing. Otherwise, if getConnection() fails quickly, you'll create lots of CPU load (and probably I/O load due to logging). – Chris Lercher Feb 17 '11 at 21:20

2 Answers2

5

This is fine as far as the executor goes.

But keep in mind that failures may occur pretty quickly, and then the Executor may re-run your code quickly. This can result in burning a lot of CPU for no results.

Build in forced retry delays and maximum loop counts.

Darron
  • 21,309
  • 5
  • 49
  • 53
4

  1. There is a risk of so called poison message to occur: the task will repeat itself infinitely if the SQLException won't get away. You must provide some sort of counter or timer.

  2. Depending on the occupancy of the executor (how many concurrent tasks are already scheduled), the interval between retries may vary significantly. You might either use 100% of the CPU or wait for retry for a very long time.

  3. If, by incident, your parent task (the one that re-schedules itself) waits for the result of the child (re-scheduled) invocation, you might experience deadlock when the executor runs on only one thread.

  4. You are using raw fields of MyApp, seems like a bad pattern whatsoever.

As for the overall idea: why not just have a loop in run()? You want to be more "fair" to other tasks being executed?

Tomasz Nurkiewicz
  • 334,321
  • 69
  • 703
  • 674