From 0e0c0a4093eeda85c76aafc9497bd357ec12e26c Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sat, 30 Apr 2022 15:06:15 +0200 Subject: [PATCH 1/3] [tcp] Fix handling in connection exceptions when resuming a stream Smack would previous run into "assert smResumptionFailed != null;" at line 407, since if a connection exception was encountered, waitForConditionOrConnectionException() would return, but we afterards just assumed that either SM resumption was successful or not. --- .../main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java index e8de5e131..c7925ce50 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java @@ -395,7 +395,7 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { if (isSmResumptionPossible()) { smResumedSyncPoint = SyncPointState.request_sent; sendNonza(new Resume(clientHandledStanzasCount, smSessionId)); - waitForConditionOrConnectionException(() -> smResumedSyncPoint == SyncPointState.successful || smResumptionFailed != null, "resume previous stream"); + waitForConditionOrThrowConnectionException(() -> smResumedSyncPoint == SyncPointState.successful || smResumptionFailed != null, "resume previous stream"); if (smResumedSyncPoint == SyncPointState.successful) { // We successfully resumed the stream, be done here afterSuccessfulLogin(true); From 1f5326abb215a7a3c593f3bbaf26607a62d07f05 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sat, 30 Apr 2022 15:08:44 +0200 Subject: [PATCH 2/3] [core] Inline waitForConditionOrConnectionException Using any of the two methods is error prone, see 0e0c0a4093ee ("[tcp] Fix handling in connection exceptions when resuming a stream"), as one can easily forget to check for connection exceptions after it returned. There are also no longer any call sites of those methods. Let's inline both variants of this method. --- .../smack/AbstractXMPPConnection.java | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java index 3f913120e..116f3b15d 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -691,7 +691,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { } /** - * We use an extra object for {@link #notifyWaitingThreads()} and {@link #waitForConditionOrConnectionException(Supplier)}, because all state + * We use an extra object for {@link #notifyWaitingThreads()} and {@link #waitFor(Supplier)}, because all state * changing methods of the connection are synchronized using the connection instance as monitor. If we now would * also use the connection instance for the internal process to wait for a condition, the {@link Object#wait()} * would leave the monitor when it waites, which would allow for another potential call to a state changing function @@ -719,22 +719,18 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { return true; } - protected final boolean waitForConditionOrConnectionException(Supplier condition) throws InterruptedException { - return waitFor(() -> condition.get().booleanValue() || hasCurrentConnectionException()); - } - - protected final void waitForConditionOrConnectionException(Supplier condition, String waitFor) throws InterruptedException, NoResponseException { - boolean success = waitForConditionOrConnectionException(condition); - if (!success) { - throw NoResponseException.newWith(this, waitFor); - } - } - protected final void waitForConditionOrThrowConnectionException(Supplier condition, String waitFor) throws InterruptedException, SmackException, XMPPException { - waitForConditionOrConnectionException(condition, waitFor); + boolean success = waitFor(() -> condition.get().booleanValue() || hasCurrentConnectionException()); if (hasCurrentConnectionException()) { throwCurrentConnectionException(); } + + // If there was no connection exception and we still did not successfully wait for the condition to hold, then + // we ran into a no-response timeout. + if (!success) { + throw NoResponseException.newWith(this, waitFor); + } + // Otherwise we successfully awaited the condition. } protected Resourcepart bindResourceAndEstablishSession(Resourcepart resource) From 881ebe731d720e407ef626d67ae35c6b02d6c60d Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sat, 30 Apr 2022 15:15:51 +0200 Subject: [PATCH 3/3] [core] Fix typo in javadoc --- .../java/org/jivesoftware/smack/AbstractXMPPConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java index 116f3b15d..294cd87c2 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -694,7 +694,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { * We use an extra object for {@link #notifyWaitingThreads()} and {@link #waitFor(Supplier)}, because all state * changing methods of the connection are synchronized using the connection instance as monitor. If we now would * also use the connection instance for the internal process to wait for a condition, the {@link Object#wait()} - * would leave the monitor when it waites, which would allow for another potential call to a state changing function + * would leave the monitor when it waits, which would allow for another potential call to a state changing function * to proceed. */ private final Object internalMonitor = new Object();