From f1e24e227367f3ec6723d2fd2c1a855c9b287017 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 31 Jul 2016 14:30:31 +0200 Subject: [PATCH] Rework Roster's SubscribeListener allow multiple of them to be installed, instead of at most one. Fixes deadlock in LowLevelRosterIntegration test because IoTProvisioningManager's SubscribeListener would not come up with a decission. --- .../provisioning/IoTProvisioningManager.java | 2 +- .../org/jivesoftware/smack/roster/Roster.java | 47 ++++++++++++++----- .../smack/roster/RosterIntegrationTest.java | 22 ++++++--- 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/smack-experimental/src/main/java/org/jivesoftware/smackx/iot/provisioning/IoTProvisioningManager.java b/smack-experimental/src/main/java/org/jivesoftware/smackx/iot/provisioning/IoTProvisioningManager.java index 077a06882..d9264dfca 100644 --- a/smack-experimental/src/main/java/org/jivesoftware/smackx/iot/provisioning/IoTProvisioningManager.java +++ b/smack-experimental/src/main/java/org/jivesoftware/smackx/iot/provisioning/IoTProvisioningManager.java @@ -151,7 +151,7 @@ public final class IoTProvisioningManager extends Manager { }); roster = Roster.getInstanceFor(connection); - roster.setSubscribeListener(new SubscribeListener() { + roster.addSubscribeListener(new SubscribeListener() { @Override public SubscribeAnswer processSubscribe(Jid from, Presence subscribeRequest) { // First check if the subscription request comes from a known registry and accept the request if so. diff --git a/smack-im/src/main/java/org/jivesoftware/smack/roster/Roster.java b/smack-im/src/main/java/org/jivesoftware/smack/roster/Roster.java index 59513f454..52a18dad2 100644 --- a/smack-im/src/main/java/org/jivesoftware/smack/roster/Roster.java +++ b/smack-im/src/main/java/org/jivesoftware/smack/roster/Roster.java @@ -199,7 +199,9 @@ public final class Roster extends Manager { private SubscriptionMode subscriptionMode = getDefaultSubscriptionMode(); - private SubscribeListener subscribeListener; + private final Set subscribeListeners = new CopyOnWriteArraySet<>(); + + private SubscriptionMode previousSubscriptionMode; /** * Returns the default subscription processing mode to use when a new Roster is created. The @@ -249,11 +251,12 @@ public final class Roster extends Manager { SubscribeAnswer subscribeAnswer = null; switch (subscriptionMode) { case manual: - final SubscribeListener subscribeListener = Roster.this.subscribeListener; - if (subscribeListener == null) { - return; + for (SubscribeListener subscribeListener : subscribeListeners) { + subscribeAnswer = subscribeListener.processSubscribe(from, presence); + if (subscribeAnswer != null) { + break; + } } - subscribeAnswer = subscribeListener.processSubscribe(from, presence); if (subscribeAnswer == null) { return; } @@ -666,19 +669,37 @@ public final class Roster extends Manager { } /** - * Set the subscribe listener, which is invoked on incoming subscription requests and if - * {@link SubscriptionMode} is set to {@link SubscriptionMode#manual}. If - * subscribeListener is not null, then this also sets subscription + * Add a subscribe listener, which is invoked on incoming subscription requests and if + * {@link SubscriptionMode} is set to {@link SubscriptionMode#manual}. This also sets subscription * mode to {@link SubscriptionMode#manual}. * - * @param subscribeListener the subscribe listener to set. + * @param subscribeListener the subscribe listener to add. + * @return true if the listener was not already added. * @since 4.2 */ - public void setSubscribeListener(SubscribeListener subscribeListener) { - if (subscribeListener != null) { - setSubscriptionMode(SubscriptionMode.manual); + public boolean addSubscribeListener(SubscribeListener subscribeListener) { + Objects.requireNonNull(subscribeListener, "SubscribeListener argument must not be null"); + if (subscriptionMode != SubscriptionMode.manual) { + previousSubscriptionMode = subscriptionMode; } - this.subscribeListener = subscribeListener; + return subscribeListeners.add(subscribeListener); + } + + /** + * Remove a subscribe listener. Also restores the previous subscription mode + * state, if the last listener got removed. + * + * @param subscribeListener + * the subscribe listener to remove. + * @return true if the listener registered and got removed. + * @since 4.2 + */ + public boolean removeSubscribeListener(SubscribeListener subscribeListener) { + boolean removed = subscribeListeners.remove(subscribeListener); + if (removed && subscribeListeners.isEmpty()) { + setSubscriptionMode(previousSubscriptionMode); + } + return removed; } /** diff --git a/smack-integration-test/src/main/java/org/jivesoftware/smack/roster/RosterIntegrationTest.java b/smack-integration-test/src/main/java/org/jivesoftware/smack/roster/RosterIntegrationTest.java index d5a6394d3..8691bacac 100644 --- a/smack-integration-test/src/main/java/org/jivesoftware/smack/roster/RosterIntegrationTest.java +++ b/smack-integration-test/src/main/java/org/jivesoftware/smack/roster/RosterIntegrationTest.java @@ -50,7 +50,7 @@ public class RosterIntegrationTest extends AbstractSmackIntegrationTest { public void subscribeRequestListenerTest() throws TimeoutException, Exception { ensureBothAccountsAreNotInEachOthersRoster(conOne, conTwo); - rosterTwo.setSubscribeListener(new SubscribeListener() { + final SubscribeListener subscribeListener = new SubscribeListener() { @Override public SubscribeAnswer processSubscribe(Jid from, Presence subscribeRequest) { if (from.equals(conOne.getUser().asBareJid())) { @@ -58,7 +58,8 @@ public class RosterIntegrationTest extends AbstractSmackIntegrationTest { } return SubscribeAnswer.Deny; } - }); + }; + rosterTwo.addSubscribeListener(subscribeListener); final String conTwosRosterName = "ConTwo " + testRunId; final SimpleResultSyncPoint addedAndSubscribed = new SimpleResultSyncPoint(); @@ -88,9 +89,15 @@ public class RosterIntegrationTest extends AbstractSmackIntegrationTest { } } }); - rosterOne.createEntry(conTwo.getUser().asBareJid(), conTwosRosterName, null); - assertTrue(addedAndSubscribed.waitForResult(2 * connection.getPacketReplyTimeout())); + try { + rosterOne.createEntry(conTwo.getUser().asBareJid(), conTwosRosterName, null); + + assertTrue(addedAndSubscribed.waitForResult(2 * connection.getPacketReplyTimeout())); + } + finally { + rosterTwo.removeSubscribeListener(subscribeListener); + } } public static void ensureBothAccountsAreNotInEachOthersRoster(XMPPConnection conOne, XMPPConnection conTwo) throws NotLoggedInException, @@ -124,7 +131,7 @@ public class RosterIntegrationTest extends AbstractSmackIntegrationTest { return; } - rosterOne.setSubscribeListener(new SubscribeListener() { + final SubscribeListener subscribeListener = new SubscribeListener() { @Override public SubscribeAnswer processSubscribe(Jid from, Presence subscribeRequest) { if (from.equals(conTwo.getUser().asBareJid())) { @@ -132,7 +139,8 @@ public class RosterIntegrationTest extends AbstractSmackIntegrationTest { } return SubscribeAnswer.Deny; } - }); + }; + rosterOne.addSubscribeListener(subscribeListener); final SimpleResultSyncPoint syncPoint = new SimpleResultSyncPoint(); rosterTwo.addPresenceEventListener(new AbstractPresenceEventListener() { @@ -149,7 +157,7 @@ public class RosterIntegrationTest extends AbstractSmackIntegrationTest { try { syncPoint.waitForResult(timeout); } finally { - rosterOne.setSubscribeListener(null); + rosterOne.removeSubscribeListener(subscribeListener); } } }