Execute startup method asynchronously

The goal is to call startManager on application start so that the manager is initialized in background without blocking application main thread.

Is it a safe implementation? Any recommendation or best practice? (use an Executor, use a synchronized block, ...)

```
public class FileConverter
{
/** The Constant logger. */
private static final Logger logger = LoggerFactory.getLogger(FileConverter.class);

private static RunnableFuture managerFuture;

/**
* Start manager.
*/
public static synchronized void startManager()
{
if(managerFuture == null)
{
Callable callable = new Callable()
{
@Override
public OfficeManager call() throws Exception
{
OfficeManagerConfiguration configuration = new OfficeManagerConfiguration();
OfficeManager officeManager = configuration.buildOfficeManager();

officeManager.start();

logger.info("officeManager started");

return officeManager;
}
};

managerFuture = new FutureTask(callable);
new Thread(managerFuture).start();
}
}

/**
* Stop manager.
*/
public static synchronized void stopManager()
{
if(managerFuture != null)
{
managerFuture.cancel(true);

try
{
OfficeManager officeManager = managerFuture.get();
if(officeManager.isRunning())
{
officeManager.stop();
}
}
catch(CancellationException e)
{
// ignore
}
catch(InterruptedException e)
{
Thread.currentThread().interrupt();
}
catch(ExecutionException e)
{
logger.warn(e.getMessage(), e);
}

There's a potential failure. If a thread invokes startManager() and then another thread immediately invokes stopManager(), stopManager() could see managerFuture as null and do nothing before startManager() gets a chance to populate it. The end result would be as if stopManager() was never called.

There's no reason for convert() to call startManager() if the manager gets started on application start.

Don't ignore InterruptedException. At the very least call Thread.currentThread().interrupt(). There are many resources out there on how to correctly handle InterruptedException, including on StackOverflow.

I also question the premise that the manager needs to start up on another thread. You should only bother if this is client-side code and this is a demonstrated bottleneck on startup.

Updated Answer

• It will, but synchronized is a blunt instrument. It usually locks for longer than you need, and it's a lock on the object itself. If somebody else used your class as a lock, none of your synchronized methods could run until it was released. If you insist on going this route, you should use a private static final RentrantLock and minimize the scope to what you really need to be locked.

• No, it shouldn't, but it doesn't do anything. If you call startManager() on startup, then the init thread is already running. If convert() also calls it, convert() will wait for the init thread to release its lock, then find out that it's already been initialized. OR you can do lazy-load of the manager in convert(), but then you have to wait for it to spin up the first time, which doesn't seem optimal if it takes 5 seconds.

• Ok.

• My point is that introducing this logic to save yourself 5 seconds on application start-up in dev is a bad tradeoff. Performance tuning is fragile and usually difficult to understand. Don't do it unless you need to. Junior programmers will get confused and muck it up by accident.

• I don't think you need a reference to the thread. You have the Future .. that's what you care about. The only operation you want to perform on the thread after you start it is to interrupt it, which you can do from the future. If you need to hold onto the thread for some reason, by all means do so, but if you don't, there's no point in keeping a pointer to it.

StackExchange Code Review Q#77818, answer score: 2