From 3dfaa2fc528611509b7de281a3d0152a5e58a498 Mon Sep 17 00:00:00 2001 From: woodser Date: Mon, 5 Aug 2024 12:09:17 -0400 Subject: [PATCH] fix concurrent modification exception in disputes list --- .../haveno/core/api/CoreDisputesService.java | 6 ++-- .../alerts/DisputeMsgEvents.java | 22 +++++++------ .../haveno/core/support/SupportManager.java | 32 +++++++++++-------- .../haveno/core/support/dispute/Dispute.java | 24 ++++++++------ .../support/dispute/DisputeListService.java | 28 +++++++++------- .../core/support/dispute/DisputeManager.java | 16 ++++++---- .../arbitration/ArbitrationManager.java | 10 +++--- 7 files changed, 80 insertions(+), 58 deletions(-) diff --git a/core/src/main/java/haveno/core/api/CoreDisputesService.java b/core/src/main/java/haveno/core/api/CoreDisputesService.java index 6f17d88f57..7edbed9b01 100644 --- a/core/src/main/java/haveno/core/api/CoreDisputesService.java +++ b/core/src/main/java/haveno/core/api/CoreDisputesService.java @@ -275,10 +275,12 @@ public class CoreDisputesService { disputeResult.summaryNotesProperty().get() ); - if (reason == DisputeResult.Reason.OPTION_TRADE && + synchronized (dispute.getChatMessages()) { + if (reason == DisputeResult.Reason.OPTION_TRADE && dispute.getChatMessages().size() > 1 && dispute.getChatMessages().get(1).isSystemMessage()) { - textToSign += "\n" + dispute.getChatMessages().get(1).getMessage() + "\n"; + textToSign += "\n" + dispute.getChatMessages().get(1).getMessage() + "\n"; + } } String summaryText = DisputeSummaryVerification.signAndApply(disputeManager, disputeResult, textToSign); diff --git a/core/src/main/java/haveno/core/notifications/alerts/DisputeMsgEvents.java b/core/src/main/java/haveno/core/notifications/alerts/DisputeMsgEvents.java index cade25305a..60a7e60ba0 100644 --- a/core/src/main/java/haveno/core/notifications/alerts/DisputeMsgEvents.java +++ b/core/src/main/java/haveno/core/notifications/alerts/DisputeMsgEvents.java @@ -114,16 +114,18 @@ public class DisputeMsgEvents { // We check at every new message if it might be a message sent after the dispute had been closed. If that is the // case we revert the isClosed flag so that the UI can reopen the dispute and indicate that a new dispute // message arrived. - ObservableList chatMessages = dispute.getChatMessages(); - // If last message is not a result message we re-open as we might have received a new message from the - // trader/mediator/arbitrator who has reopened the case - if (dispute.isClosed() && !chatMessages.isEmpty() && !chatMessages.get(chatMessages.size() - 1).isResultMessage(dispute)) { - dispute.reOpen(); - if (dispute.getSupportType() == SupportType.MEDIATION) { - mediationManager.requestPersistence(); - } else if (dispute.getSupportType() == SupportType.REFUND) { - refundManager.requestPersistence(); + synchronized (dispute.getChatMessages()) { + ObservableList chatMessages = dispute.getChatMessages(); + // If last message is not a result message we re-open as we might have received a new message from the + // trader/mediator/arbitrator who has reopened the case + if (dispute.isClosed() && !chatMessages.isEmpty() && !chatMessages.get(chatMessages.size() - 1).isResultMessage(dispute)) { + dispute.reOpen(); + if (dispute.getSupportType() == SupportType.MEDIATION) { + mediationManager.requestPersistence(); + } else if (dispute.getSupportType() == SupportType.REFUND) { + refundManager.requestPersistence(); + } } - } + } } } diff --git a/core/src/main/java/haveno/core/support/SupportManager.java b/core/src/main/java/haveno/core/support/SupportManager.java index caca60ac9f..d76154485d 100644 --- a/core/src/main/java/haveno/core/support/SupportManager.java +++ b/core/src/main/java/haveno/core/support/SupportManager.java @@ -192,17 +192,19 @@ public abstract class SupportManager { if (ackMessage.getSourceMsgClassName().equals(ChatMessage.class.getSimpleName())) { Trade trade = tradeManager.getTrade(ackMessage.getSourceId()); for (Dispute dispute : trade.getDisputes()) { - for (ChatMessage chatMessage : dispute.getChatMessages()) { - if (chatMessage.getUid().equals(ackMessage.getSourceUid())) { - if (trade.getDisputeState() == Trade.DisputeState.DISPUTE_REQUESTED) { - if (dispute.isClosed()) dispute.reOpen(); - trade.advanceDisputeState(Trade.DisputeState.DISPUTE_OPENED); - } else if (dispute.isClosed()) { - trade.pollWalletNormallyForMs(30000); // sync to check for payout + synchronized (dispute.getChatMessages()) { + for (ChatMessage chatMessage : dispute.getChatMessages()) { + if (chatMessage.getUid().equals(ackMessage.getSourceUid())) { + if (trade.getDisputeState() == Trade.DisputeState.DISPUTE_REQUESTED) { + if (dispute.isClosed()) dispute.reOpen(); + trade.advanceDisputeState(Trade.DisputeState.DISPUTE_OPENED); + } else if (dispute.isClosed()) { + trade.pollWalletNormallyForMs(30000); // sync to check for payout + } + } } } } - } } } else { log.warn("Received AckMessage with error state for {} with tradeId={}, sender={}, errorMessage={}", @@ -212,12 +214,14 @@ public abstract class SupportManager { if (ackMessage.getSourceMsgClassName().equals(ChatMessage.class.getSimpleName())) { Trade trade = tradeManager.getTrade(ackMessage.getSourceId()); for (Dispute dispute : trade.getDisputes()) { - for (ChatMessage chatMessage : dispute.getChatMessages()) { - if (chatMessage.getUid().equals(ackMessage.getSourceUid())) { - if (trade.getDisputeState().isCloseRequested()) { - log.warn("DisputeCloseMessage was nacked. We close the dispute now. tradeId={}, nack sender={}", trade.getId(), ackMessage.getSenderNodeAddress()); - dispute.setIsClosed(); - trade.advanceDisputeState(Trade.DisputeState.DISPUTE_CLOSED); + synchronized (dispute.getChatMessages()) { + for (ChatMessage chatMessage : dispute.getChatMessages()) { + if (chatMessage.getUid().equals(ackMessage.getSourceUid())) { + if (trade.getDisputeState().isCloseRequested()) { + log.warn("DisputeCloseMessage was nacked. We close the dispute now. tradeId={}, nack sender={}", trade.getId(), ackMessage.getSenderNodeAddress()); + dispute.setIsClosed(); + trade.advanceDisputeState(Trade.DisputeState.DISPUTE_CLOSED); + } } } } diff --git a/core/src/main/java/haveno/core/support/dispute/Dispute.java b/core/src/main/java/haveno/core/support/dispute/Dispute.java index cd7e13d03a..5828710d71 100644 --- a/core/src/main/java/haveno/core/support/dispute/Dispute.java +++ b/core/src/main/java/haveno/core/support/dispute/Dispute.java @@ -353,10 +353,12 @@ public final class Dispute implements NetworkPayload, PersistablePayload { /////////////////////////////////////////////////////////////////////////////////////////// public void addAndPersistChatMessage(ChatMessage chatMessage) { - if (!chatMessages.contains(chatMessage)) { - chatMessages.add(chatMessage); - } else { - log.error("disputeDirectMessage already exists"); + synchronized (chatMessages) { + if (!chatMessages.contains(chatMessage)) { + chatMessages.add(chatMessage); + } else { + log.error("disputeDirectMessage already exists"); + } } } @@ -365,13 +367,15 @@ public final class Dispute implements NetworkPayload, PersistablePayload { } public boolean removeAllChatMessages() { - if (chatMessages.size() > 1) { - // removes all chat except the initial guidelines message. - String firstMessageUid = chatMessages.get(0).getUid(); - chatMessages.removeIf((msg) -> !msg.getUid().equals(firstMessageUid)); - return true; + synchronized (chatMessages) { + if (chatMessages.size() > 1) { + // removes all chat except the initial guidelines message. + String firstMessageUid = chatMessages.get(0).getUid(); + chatMessages.removeIf((msg) -> !msg.getUid().equals(firstMessageUid)); + return true; + } + return false; } - return false; } public void maybeClearSensitiveData() { diff --git a/core/src/main/java/haveno/core/support/dispute/DisputeListService.java b/core/src/main/java/haveno/core/support/dispute/DisputeListService.java index ca8dd8a21c..2c90565614 100644 --- a/core/src/main/java/haveno/core/support/dispute/DisputeListService.java +++ b/core/src/main/java/haveno/core/support/dispute/DisputeListService.java @@ -90,12 +90,14 @@ public abstract class DisputeListService> impleme /////////////////////////////////////////////////////////////////////////////////////////// public void cleanupDisputes(@Nullable Consumer closedDisputeHandler) { - disputeList.stream().forEach(dispute -> { - String tradeId = dispute.getTradeId(); - if (dispute.isClosed() && closedDisputeHandler != null) { - closedDisputeHandler.accept(tradeId); - } - }); + synchronized (disputeList.getObservableList()) { + disputeList.stream().forEach(dispute -> { + String tradeId = dispute.getTradeId(); + if (dispute.isClosed() && closedDisputeHandler != null) { + closedDisputeHandler.accept(tradeId); + } + }); + } } @@ -130,7 +132,9 @@ public abstract class DisputeListService> impleme } ObservableList getObservableList() { - return disputeList.getObservableList(); + synchronized (disputeList.getObservableList()) { + return disputeList.getObservableList(); + } } @@ -151,10 +155,12 @@ public abstract class DisputeListService> impleme isAlerting -> { // We get the event before the list gets updated, so we execute on next frame UserThread.execute(() -> { - int numAlerts = (int) disputeList.getList().stream() - .mapToLong(x -> x.getBadgeCountProperty().getValue()) - .sum(); - numOpenDisputes.set(numAlerts); + synchronized (disputeList.getObservableList()) { + int numAlerts = (int) disputeList.getList().stream() + .mapToLong(x -> x.getBadgeCountProperty().getValue()) + .sum(); + numOpenDisputes.set(numAlerts); + } }); }); disputedTradeIds.add(dispute.getTradeId()); diff --git a/core/src/main/java/haveno/core/support/dispute/DisputeManager.java b/core/src/main/java/haveno/core/support/dispute/DisputeManager.java index 05bcfaf117..192516b59f 100644 --- a/core/src/main/java/haveno/core/support/dispute/DisputeManager.java +++ b/core/src/main/java/haveno/core/support/dispute/DisputeManager.java @@ -187,10 +187,12 @@ public abstract class DisputeManager> extends Sup @Override public List getAllChatMessages(String tradeId) { - return getDisputeList().stream() - .filter(dispute -> dispute.getTradeId().equals(tradeId)) - .flatMap(dispute -> dispute.getChatMessages().stream()) - .collect(Collectors.toList()); + synchronized (getDisputeList().getObservableList()) { + return getDisputeList().stream() + .filter(dispute -> dispute.getTradeId().equals(tradeId)) + .flatMap(dispute -> dispute.getChatMessages().stream()) + .collect(Collectors.toList()); + } } @Override @@ -239,7 +241,7 @@ public abstract class DisputeManager> extends Sup } public ObservableList getDisputesAsObservableList() { - synchronized(disputeListService.getDisputeList()) { + synchronized(disputeListService.getDisputeList().getObservableList()) { return disputeListService.getObservableList(); } } @@ -249,7 +251,7 @@ public abstract class DisputeManager> extends Sup } protected T getDisputeList() { - synchronized(disputeListService.getDisputeList()) { + synchronized(disputeListService.getDisputeList().getObservableList()) { return disputeListService.getDisputeList(); } } @@ -354,7 +356,7 @@ public abstract class DisputeManager> extends Sup return; } - synchronized (disputeList) { + synchronized (disputeList.getObservableList()) { if (disputeList.contains(dispute)) { String msg = "We got a dispute msg that we have already stored. TradeId = " + dispute.getTradeId() + ", DisputeId = " + dispute.getId(); log.warn(msg); diff --git a/core/src/main/java/haveno/core/support/dispute/arbitration/ArbitrationManager.java b/core/src/main/java/haveno/core/support/dispute/arbitration/ArbitrationManager.java index 9209ef7756..0ac6aa432b 100644 --- a/core/src/main/java/haveno/core/support/dispute/arbitration/ArbitrationManager.java +++ b/core/src/main/java/haveno/core/support/dispute/arbitration/ArbitrationManager.java @@ -287,10 +287,12 @@ public final class ArbitrationManager extends DisputeManager