Deadlock in ThreadedListenerManager.shutdown(B)

After bot.sendIRC().quitServer(), I found that the PircBotX thread would almost 
never terminate (in a few executions, it did terminate, by luck). Through 
debugging I narrowed it down to the following code:

public void shutdown(B bot) {
    synchronized (runningListeners) {
        for (ManagedFutureTask curFuture : runningListeners.get(bot))
            try {
                log.debug("Waiting for listener " + curFuture.getListener() + " to execute event " + curFuture.getEvent());
                curFuture.get();
            } catch (Exception e) {
                throw new RuntimeException("Cannot shutdown listener " + curFuture.getListener() + " executing event " + curFuture.getEvent(), e);
            }
        }
}

and in ManagedFutureTask

public ManagedFutureTask(Listener<B> listener, Event<B> event, Callable<Void> 
callable) {
    super(callable);
    this.listener = listener;
    this.event = event;
    if (event.getBot() != null)
        synchronized (runningListeners) {
            runningListeners.put(event.getBot(), this);
        }
}

@Override
protected void done() {
    if (event.getBot() != null)
        synchronized (runningListeners) {
            runningListeners.remove(event.getBot(), this);
        }
}

the locking goes like this:
1. shutdown() locks runningListeners
2. shutdown() blocks on the next Future
3. the future finishes, and tries to lock runningListeners. However this is 
still locked by shutdown()

I attempted to fix this in a subclass as follows:

    public void shutdown(B bot) {
        // We must unlock the listeners before iterating and calling .get(),
        // since .get() can also try to block on runningListeners.
        // Instead we make a copy of the list, then wait on each one. If they finish before we start
        // iterating, that's fine, since .get() will just return immediately.
        List<ManagedFutureTask> remainingTasks;
        synchronized (runningListeners) {
            remainingTasks = new ArrayList<>(runningListeners.get(bot));
        }
        for (ManagedFutureTask curFuture : remainingTasks) {
            try {
                log.debug("Waiting for listener " + curFuture.getListener() +
                          " to execute event " + curFuture.getEvent());
                curFuture.get();
            } catch (Exception e) {
                throw new RuntimeException(
                        "Cannot shutdown listener " + curFuture.getListener() +
                        " executing event " + curFuture.getEvent(), e);
            }
        }
    }

Essentially by making a copy of the remaining runningListeners, we avoid having 
to lock the list for the entire loop. Since I am new to the code, I make the 
assumption that no FutureTasks may be added after shutdown() has started.
If a task is removed by the time we start iterating, calling .get() shouldn't 
cause any problems.
Original issue reported on code.google.com by mikebuck...@gmail.com on 30 Aug 2014 at 10:34
raisedragon / pircbotx

Deadlock in ThreadedListenerManager.shutdown(B) #202
