From 1aad56595f8c73384cd8c04ca154270b9c26159a Mon Sep 17 00:00:00 2001 From: Roel Rymenants Date: Fri, 3 Mar 2017 11:21:01 +0100 Subject: [PATCH] Reverse lock order to avoid deadlock In class AutorecoveringConnection: maybeDeleteRecordedAutoDeleteExchange takes the locks on - recordedExchanges - consumers maybeDeleteRecordedAutoDeleteQueue takes the locks on - recordedQueues - consumers Since the latter one also calls the former, the following deadlock may occur Thread1: maybeDeleteRecordedAutoDeleteExchange - locked recordedExchanges - waiting for consumers (locked by thread2) Thread2: maybeDeleteRecordedAutoDeleteQueue - locked recordedQueues - locked consumers maybeDeleteRecordedAutoDeleteExchange - waiting for recordedExchanges (locked by thread1) By reversing the locks, both flows will first lock consumers, avoiding taking other locks in a different order --- .../client/impl/recovery/AutorecoveringConnection.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/rabbitmq/client/impl/recovery/AutorecoveringConnection.java b/src/main/java/com/rabbitmq/client/impl/recovery/AutorecoveringConnection.java index 7d8f09545a..c471cf3125 100644 --- a/src/main/java/com/rabbitmq/client/impl/recovery/AutorecoveringConnection.java +++ b/src/main/java/com/rabbitmq/client/impl/recovery/AutorecoveringConnection.java @@ -772,8 +772,8 @@ RecordedConsumer deleteRecordedConsumer(String consumerTag) { } void maybeDeleteRecordedAutoDeleteQueue(String queue) { - synchronized (this.recordedQueues) { - synchronized (this.consumers) { + synchronized (this.consumers) { + synchronized (this.recordedQueues) { if(!hasMoreConsumersOnQueue(this.consumers.values(), queue)) { RecordedQueue q = this.recordedQueues.get(queue); // last consumer on this connection is gone, remove recorded queue @@ -787,8 +787,8 @@ void maybeDeleteRecordedAutoDeleteQueue(String queue) { } void maybeDeleteRecordedAutoDeleteExchange(String exchange) { - synchronized (this.recordedExchanges) { - synchronized (this.consumers) { + synchronized (this.consumers) { + synchronized (this.recordedExchanges) { if(!hasMoreDestinationsBoundToExchange(Utility.copy(this.recordedBindings), exchange)) { RecordedExchange x = this.recordedExchanges.get(exchange); // last binding where this exchange is the source is gone, remove recorded exchange