From 1bd3469fecbd0daa7cce2973d9be55125ec00e7a Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Thu, 22 Feb 2018 15:05:12 +0100 Subject: [PATCH 1/7] Also set persist items to true in PubSubIntegrationTest --- .../org/jivesoftware/smackx/pubsub/PubSubIntegrationTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/smack-integration-test/src/main/java/org/jivesoftware/smackx/pubsub/PubSubIntegrationTest.java b/smack-integration-test/src/main/java/org/jivesoftware/smackx/pubsub/PubSubIntegrationTest.java index 403535d9f..a8f19b6c5 100644 --- a/smack-integration-test/src/main/java/org/jivesoftware/smackx/pubsub/PubSubIntegrationTest.java +++ b/smack-integration-test/src/main/java/org/jivesoftware/smackx/pubsub/PubSubIntegrationTest.java @@ -58,6 +58,7 @@ public class PubSubIntegrationTest extends AbstractSmackIntegrationTest { // items do not need payload, to prevent payload-required error responses when // publishing the item. config.setDeliverPayloads(false); + config.setPersistentItems(true); Node node = pubSubManagerOne.createNode(nodename, config); try { LeafNode leafNode = (LeafNode) node; From 5e25491877b8604d9d53b07c376068f392ba6191 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Tue, 20 Feb 2018 09:44:05 +0100 Subject: [PATCH 2/7] Do not send unavailable on disconnect() when not authenticated --- .../smack/AbstractXMPPConnection.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 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 5bb64c3b5..6f3de549c 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -706,8 +706,12 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { * */ public void disconnect() { + Presence unavailablePresence = null; + if (isAuthenticated()) { + unavailablePresence = new Presence(Presence.Type.unavailable); + } try { - disconnect(new Presence(Presence.Type.unavailable)); + disconnect(unavailablePresence); } catch (NotConnectedException e) { LOGGER.log(Level.FINEST, "Connection is already disconnected", e); @@ -722,15 +726,18 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { * stanza(/packet) is set with online information, but most XMPP servers will deliver the full * presence stanza(/packet) with whatever data is set. * - * @param unavailablePresence the presence stanza(/packet) to send during shutdown. + * @param unavailablePresence the optional presence stanza to send during shutdown. * @throws NotConnectedException */ public synchronized void disconnect(Presence unavailablePresence) throws NotConnectedException { - try { - sendStanza(unavailablePresence); - } - catch (InterruptedException e) { - LOGGER.log(Level.FINE, "Was interrupted while sending unavailable presence. Continuing to disconnect the connection", e); + if (unavailablePresence != null) { + try { + sendStanza(unavailablePresence); + } catch (InterruptedException e) { + LOGGER.log(Level.FINE, + "Was interrupted while sending unavailable presence. Continuing to disconnect the connection", + e); + } } shutdown(); callConnectionClosedListener(); From 793d3c47ad8eb00fb5cbe70afa2fa1b41992dc3f Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Tue, 20 Feb 2018 09:01:11 +0100 Subject: [PATCH 3/7] Move TLS and SASL sync points into subclasses In preperation of subclasses with different connection approaches. --- .../org/jivesoftware/smack/bosh/XMPPBOSHConnection.java | 3 --- .../java/org/jivesoftware/smack/AbstractXMPPConnection.java | 6 ------ .../test/java/org/jivesoftware/smack/DummyConnection.java | 2 -- .../java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java | 6 ++++++ 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java b/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java index 95fd3bc6c..ed44debf3 100644 --- a/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java +++ b/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java @@ -201,9 +201,6 @@ public class XMPPBOSHConnection extends AbstractXMPPConnection { + getHost() + ":" + getPort() + "."; throw new SmackException(errorMessage); } - - tlsHandled.reportSuccess(); - saslFeatureReceived.reportSuccess(); } @Override 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 6f3de549c..78855474a 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -379,12 +379,6 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { // Perform the actual connection to the XMPP service connectInternal(); - // TLS handled will be successful either if TLS was established, or if it was not mandatory. - tlsHandled.checkIfSuccessOrWaitOrThrow(); - - // Wait with SASL auth until the SASL mechanisms have been received - saslFeatureReceived.checkIfSuccessOrWaitOrThrow(); - // If TLS is required but the server doesn't offer it, disconnect // from the server and throw an error. First check if we've already negotiated TLS // and are secure, however (features get parsed a second time after TLS is established). diff --git a/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java b/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java index e5351835e..ab4a43bff 100644 --- a/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java +++ b/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java @@ -92,8 +92,6 @@ public class DummyConnection extends AbstractXMPPConnection { @Override protected void connectInternal() { connected = true; - saslFeatureReceived.reportSuccess(); - tlsHandled.reportSuccess(); streamId = "dummy-" + new Random(new Date().getTime()).nextInt(); // TODO: Remove in Smack 4.3, and likely the suppression of the deprecation warning. 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 c1830a03e..e6db31378 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 @@ -903,6 +903,12 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { // We connected successfully to the servers TCP port initConnection(); + + // TLS handled will be successful either if TLS was established, or if it was not mandatory. + tlsHandled.checkIfSuccessOrWaitOrThrow(); + + // Wait with SASL auth until the SASL mechanisms have been received + saslFeatureReceived.checkIfSuccessOrWaitOrThrow(); } /** From 20014d56b42ccde9e05cbd5a77f6e59dcbb50a00 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Fri, 23 Feb 2018 11:50:54 +0100 Subject: [PATCH 4/7] Use true/false instead of 1/0 in boolean type FormFields --- .../src/main/java/org/jivesoftware/smackx/xdata/Form.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/Form.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/Form.java index 39e639304..63c8e5b5b 100644 --- a/smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/Form.java +++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/Form.java @@ -235,7 +235,7 @@ public class Form { if (field.getType() != FormField.Type.bool) { throw new IllegalArgumentException("This field is not of type boolean."); } - setAnswer(field, (value ? "1" : "0")); + setAnswer(field, Boolean.toString(value)); } /** From abdfe7300609d24729d192d616fdb231e6dcdabf Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Fri, 23 Feb 2018 16:48:15 +0100 Subject: [PATCH 5/7] Remove executorService from AbstractXMPPConnection Reduces thread count per connection by one. --- .../smack/AbstractXMPPConnection.java | 22 ++----- .../smack/util/BoundedThreadPoolExecutor.java | 63 ------------------- 2 files changed, 6 insertions(+), 79 deletions(-) delete mode 100644 smack-core/src/main/java/org/jivesoftware/smack/util/BoundedThreadPoolExecutor.java 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 78855474a..6bbf7c8b0 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -76,7 +76,6 @@ import org.jivesoftware.smack.parsing.ParsingExceptionCallback; import org.jivesoftware.smack.provider.ExtensionElementProvider; import org.jivesoftware.smack.provider.ProviderManager; import org.jivesoftware.smack.sasl.core.SASLAnonymous; -import org.jivesoftware.smack.util.BoundedThreadPoolExecutor; import org.jivesoftware.smack.util.DNSUtil; import org.jivesoftware.smack.util.Objects; import org.jivesoftware.smack.util.PacketParserUtils; @@ -233,14 +232,6 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { private ParsingExceptionCallback parsingExceptionCallback = SmackConfiguration.getDefaultParsingExceptionCallback(); - /** - * ExecutorService used to invoke the PacketListeners on newly arrived and parsed stanzas. It is - * important that we use a single threaded ExecutorService in order to guarantee that the - * PacketListeners are invoked in the same order the stanzas arrived. - */ - private final BoundedThreadPoolExecutor executorService = new BoundedThreadPoolExecutor(1, 1, 0, TimeUnit.SECONDS, - 100, new SmackExecutorThreadFactory(this, "Incoming Processor")); - /** * This scheduled thread pool executor is used to remove pending callbacks. */ @@ -1081,17 +1072,17 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { assert (stanza != null); lastStanzaReceived = System.currentTimeMillis(); // Deliver the incoming packet to listeners. - executorService.executeBlocking(new Runnable() { - @Override - public void run() { - invokeStanzaCollectorsAndNotifyRecvListeners(stanza); - } - }); + invokeStanzaCollectorsAndNotifyRecvListeners(stanza); } /** * Invoke {@link StanzaCollector#processStanza(Stanza)} for every * StanzaCollector with the given packet. Also notify the receive listeners with a matching stanza(/packet) filter about the packet. + *

+ * This method will be invoked by the connections incoming processing thread which may be shared across multiple connections and + * thus it is important that no user code, e.g. in form of a callback, is invoked by this method. For the same reason, + * this method must not block for an extended period of time. + *

* * @param packet the stanza(/packet) to notify the StanzaCollectors and receive listeners about. */ @@ -1413,7 +1404,6 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { // reference to their ExecutorService which prevents the ExecutorService from being // gc'ed. It is possible that the XMPPConnection instance is gc'ed while the // listenerExecutor ExecutorService call not be gc'ed until it got shut down. - executorService.shutdownNow(); cachedExecutorService.shutdown(); removeCallbacksService.shutdownNow(); singleThreadedExecutorService.shutdownNow(); diff --git a/smack-core/src/main/java/org/jivesoftware/smack/util/BoundedThreadPoolExecutor.java b/smack-core/src/main/java/org/jivesoftware/smack/util/BoundedThreadPoolExecutor.java deleted file mode 100644 index c09ddd5d5..000000000 --- a/smack-core/src/main/java/org/jivesoftware/smack/util/BoundedThreadPoolExecutor.java +++ /dev/null @@ -1,63 +0,0 @@ -/** - * - * Copyright 2015 Florian Schmaus - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.jivesoftware.smack.util; - -import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.RejectedExecutionException; -import java.util.concurrent.Semaphore; -import java.util.concurrent.ThreadFactory; -import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.TimeUnit; - -public class BoundedThreadPoolExecutor extends ThreadPoolExecutor { - - private final Semaphore semaphore; - - public BoundedThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, - TimeUnit unit, int bound, ThreadFactory threadFactory) { - // One could think that the array blocking queue bound should be "bound - 1" because the bound protected by the - // Semaphore also includes the "slot" in the worker thread executing the Runnable. But using that as bound could - // actually cause a RejectedExecutionException as the queue could fill up while the worker thread remains - // unscheduled and is thus not removing any entries. - super(corePoolSize, maximumPoolSize, keepAliveTime, - unit, new ArrayBlockingQueue(bound), threadFactory); - semaphore = new Semaphore(bound); - } - - public void executeBlocking(final Runnable command) throws InterruptedException { - semaphore.acquire(); - try { - execute(new Runnable() { - @Override - public void run() { - try { - command.run(); - } finally { - semaphore.release(); - } - } - }); - } catch (Exception e) { - semaphore.release(); - if (e instanceof RejectedExecutionException) { - throw (RejectedExecutionException) e; - } else { - throw new RuntimeException(e); - } - } - } -} From a4ab6245f612656738366a9cc61ee6735766f1fb Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Fri, 23 Feb 2018 17:04:51 +0100 Subject: [PATCH 6/7] Make cached executor service static --- .../smack/AbstractXMPPConnection.java | 25 +++++++++---------- 1 file changed, 12 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 6bbf7c8b0..fb72a4ba9 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -33,6 +33,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.Lock; @@ -242,16 +243,15 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { * A cached thread pool executor service with custom thread factory to set meaningful names on the threads and set * them 'daemon'. */ - private final ExecutorService cachedExecutorService = Executors.newCachedThreadPool( - // @formatter:off - // CHECKSTYLE:OFF - new SmackExecutorThreadFactory( // threadFactory - this, - "Cached Executor" - ) - // @formatter:on - // CHECKSTYLE:ON - ); + private static final ExecutorService CACHED_EXECUTOR_SERVICE = Executors.newCachedThreadPool(new ThreadFactory() { + @Override + public Thread newThread(Runnable runnable) { + Thread thread = new Thread(runnable); + thread.setName("Smack Cached Executor"); + thread.setDaemon(true); + return thread; + } + }); /** * A executor service used to invoke the callbacks of synchronous stanza(/packet) listeners. We use a executor service to @@ -1141,7 +1141,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { executorService = singleThreadedExecutorService; break; case async: - executorService = cachedExecutorService; + executorService = CACHED_EXECUTOR_SERVICE; break; } final IQRequestHandler finalIqRequestHandler = iqRequestHandler; @@ -1404,7 +1404,6 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { // reference to their ExecutorService which prevents the ExecutorService from being // gc'ed. It is possible that the XMPPConnection instance is gc'ed while the // listenerExecutor ExecutorService call not be gc'ed until it got shut down. - cachedExecutorService.shutdown(); removeCallbacksService.shutdownNow(); singleThreadedExecutorService.shutdownNow(); } catch (Throwable t) { @@ -1681,7 +1680,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { } protected final void asyncGo(Runnable runnable) { - cachedExecutorService.execute(runnable); + CACHED_EXECUTOR_SERVICE.execute(runnable); } protected final ScheduledFuture schedule(Runnable runnable, long delay, TimeUnit unit) { From 72a201457207610ef2a5aafe0cd83e015cdde104 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Fri, 23 Feb 2018 18:22:53 +0100 Subject: [PATCH 7/7] Set core-pool size of single-threaded executor to zero in AbstractXMPPConnection. --- .../org/jivesoftware/smack/AbstractXMPPConnection.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 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 fb72a4ba9..8c589db99 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -31,9 +31,11 @@ import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ThreadFactory; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.Lock; @@ -258,8 +260,9 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { * decouple incoming stanza processing from callback invocation. It is important that order of callback invocation * is the same as the order of the incoming stanzas. Therefore we use a single threaded executor service. */ - private final ExecutorService singleThreadedExecutorService = Executors.newSingleThreadExecutor(new SmackExecutorThreadFactory( - this, "Single Threaded Executor")); + private final ExecutorService singleThreadedExecutorService = new ThreadPoolExecutor(0, 1, 30L, + TimeUnit.SECONDS, new LinkedBlockingQueue(), + new SmackExecutorThreadFactory(this, "Single Threaded Executor")); /** * The used host to establish the connection to