channel.queueDelete sometimes throws NullPointerException, possibly due to race on recordedBindings

[spatula75]

When multiple channels share an AutorecoveringConnection and queues are coming and going from multiple theads, about 1 time out of 10000 , we are seeing a NullPointerException or ConcurrentModificationException to be thrown:

java.lang.NullPointerException
        at com.rabbitmq.client.impl.recovery.AutorecoveringConnection.removeBindingsWithDestination(AutorecoveringConnection.java:768)
        at com.rabbitmq.client.impl.recovery.AutorecoveringConnection.deleteRecordedQueue(AutorecoveringConnection.java:690)
        at com.rabbitmq.client.impl.recovery.AutorecoveringChannel.deleteRecordedQueue(AutorecoveringChannel.java:562)
        at com.rabbitmq.client.impl.recovery.AutorecoveringChannel.queueDelete(AutorecoveringChannel.java:303)
        at com.rabbitmq.client.impl.recovery.AutorecoveringChannel.queueDelete(AutorecoveringChannel.java:299)

The code at this location looks innocuous enough:

    Set<RecordedBinding> removeBindingsWithDestination(String s) {
        Set<RecordedBinding> result = new HashSet<RecordedBinding>();
        for (Iterator<RecordedBinding> it = this.recordedBindings.iterator(); it.hasNext(); ) {
            RecordedBinding b = it.next();
            if(b.getDestination().equals(s)) { // [[ NPE here ]] //
                it.remove();
                result.add(b);
            }
        }
        return result;
    }

However, removeBindingsWithDestination is not synchronized, so it's possible for multiple threads to be in this method changing the recordedBindings ArrayList at the same time as bindings come and go from multiple threads, even with channels not being shared across threads (but the Connection is shared).

It looks the other accesses to recordedBindings are synchronized (though caution: maybeDeleteRecordedAutoDeleteExchange is synchronizing differently than the others) so removeBindingsWithDestination may have just fallen through the cracks.

It may be a sufficient fix just to make removeBindingsWithDestination synchronized.

(Another tangential thing I noticed is that most of the recorded maintenance is done from synchronized methods, but maybeDeleteRecordedAutoDeleteQueue during a basicCancel instead synchronizes on recordedQueues and consumers, and that this method just removes the recordedQueues entry, not the recordedBindings associated with the queue as well.)

[spatula75]

Possible workaround for now is to add synchronization on the channel's Connection around calls to queueDelete:

synchronized(channel.getConnection() {
    ...
    channel.queueDelete(queue);
}

This should have the same effect as though removeBindingsWithDestination in Connection were a synchronized method.

[michaelklishin]

It's hard to suggest much without knowing what exactly causes a channel to be shared. I'm OK with making removeBindingsWithDestination synchornised— feel free to submit a PR against thestable` branch. However, the client does claim to support concurrent channel use for every case. We've been pretty clear about this for years.

[spatula75]

The concurrency issue isn't within the channel. It's within the AutorecoveringConnection. It modifies recordedBindings with correct synchronization in most places, except from removeBindingsWithDestination.

[michaelklishin]

OK, sounds like an obvious fix then. Thank you for for digging it out!

[spatula75]

(Incidentally, My initial idea for a workaround is no good because channel.getConnection() would return the channel delegate's idea of a Connection, which is a different object (presumably) from the AutorecoveringConnection. But it should be possible to snag the AutorecoveringConnection with reflection and then synchronize on it to do the queueDelete, taking advantage of a synchronized method of connection being the same thing as doing synchronized(connection){} around a block. I'm going to give it a shot anyway and see what happens.)

[michaelklishin]

@spatula75 all declared entities state is managed by AutorecoveringConnection. I'd prefer extending it as needed over using reflection.

[spatula75]

Oh good point, I didn't think of just extending it!

[spatula75]

Edited the problem description for clarity and included the proposed fix. (I was sort of writing the issue as I was continuing to dig deeper, and as a result it became a bit rambling and hard to understand/follow.)

[spatula75]

Confirmed that synchronizing that method will resolve the issue; no NPEs or ConcurrentModificationExceptions in over a day, when previously we were getting them every few hours.

[michaelklishin]

@spatula75 please submit a PR (against stable)?

[spatula75]

Pardon my ignorance; what's the preferred method of submitting a PR? I haven't had a need of doing such a thing yet.