From 5d46e281fcbd60f34bd96ccbe36faa8072d39354 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Mon, 25 Mar 2019 13:06:12 +0100 Subject: [PATCH 1/3] XMPPTCPConnection log when reader/writer threads start and exit --- .../jivesoftware/smack/tcp/XMPPTCPConnection.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 97e0e2044..2441845c6 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 @@ -1063,6 +1063,8 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { protected class PacketReader { + private final String threadName = "Smack Reader (" + getConnectionCounter() + ')'; + XmlPullParser parser; private volatile boolean done; @@ -1077,13 +1079,15 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { Async.go(new Runnable() { @Override public void run() { + LOGGER.finer(threadName + " start"); try { parsePackets(); } finally { + LOGGER.finer(threadName + " exit"); XMPPTCPConnection.this.readerWriterSemaphore.release(); } } - }, "Smack Reader (" + getConnectionCounter() + ")"); + }, threadName); } /** @@ -1336,6 +1340,8 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { protected class PacketWriter { public static final int QUEUE_SIZE = XMPPTCPConnection.QUEUE_SIZE; + private final String threadName = "Smack Writer (" + getConnectionCounter() + ')'; + private final ArrayBlockingQueueWithShutdown queue = new ArrayBlockingQueueWithShutdown<>( QUEUE_SIZE, true); @@ -1381,13 +1387,15 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { Async.go(new Runnable() { @Override public void run() { + LOGGER.finer(threadName + " start"); try { writePackets(); } finally { + LOGGER.finer(threadName + " exit"); XMPPTCPConnection.this.readerWriterSemaphore.release(); } } - }, "Smack Writer (" + getConnectionCounter() + ")"); + }, threadName); } private boolean done() { From 25b3f354216683e568a54b93be1c6a931cd79b26 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Thu, 28 Mar 2019 13:52:17 +0100 Subject: [PATCH 2/3] Ensure that shutdown() terminates reader/writer threads In case an exception happens in connect()/login() the 'disconnectedButResumable' boolean may (still) be set. Which causes only one of the reader and writer threads to exit, typically the reader thread, because shutdown() will bail out very early. This leaves a dangling (writer) thread causing memory leaks and deadlocks on a subsequent connect()/login(). --- .../smack/tcp/XMPPTCPConnection.java | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) 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 2441845c6..a08221ddb 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 @@ -508,10 +508,6 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { } private void shutdown(boolean instant) { - if (disconnectedButResumeable) { - return; - } - // First shutdown the writer, this will result in a closing stream element getting send to // the server LOGGER.finer("PacketWriter shutdown()"); @@ -534,13 +530,25 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { packetReader.shutdown(); LOGGER.finer("PacketReader has been shut down"); - try { + final Socket socket = this.socket; + if (socket != null && socket.isConnected()) { + try { socket.close(); - } catch (Exception e) { + } catch (Exception e) { LOGGER.log(Level.WARNING, "shutdown", e); + } } setWasAuthenticated(); + + // Wait for reader and writer threads to be terminated. + readerWriterSemaphore.acquireUninterruptibly(2); + readerWriterSemaphore.release(2); + + if (disconnectedButResumeable) { + return; + } + // If we are able to resume the stream, then don't set // connected/authenticated/usingTLS to false since we like behave like we are still // connected (e.g. sendStanza should not throw a NotConnectedException). @@ -561,10 +569,6 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { writer = null; initState(); - - // Wait for reader and writer threads to be terminated. - readerWriterSemaphore.acquireUninterruptibly(2); - readerWriterSemaphore.release(2); } @Override From 9be498c440e2b10cae4d01b4d0037bfdb90c85cf Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Mon, 1 Apr 2019 22:02:36 +0200 Subject: [PATCH 3/3] Fix NPE in Roster's presence listeners if 'from' is not set The NPE is caused by an inbound presence stanza without the 'from' attribute set. The stacktrace of the NPE is: FATAL EXCEPTION: Smack Cached Executor Process: de.fhg.ivi.senetz.mobile.android.mbk.debug, PID: 13365 java.lang.NullPointerException: Attempt to invoke virtual method 'int java.lang.Object.hashCode()' on a null object reference at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:944) at org.jivesoftware.smack.roster.Roster.getPresencesInternal(Roster.java:374) at org.jivesoftware.smack.roster.Roster.getOrCreatePresencesInternal(Roster.java:388) at org.jivesoftware.smack.roster.Roster.access$1100(Roster.java:94) at org.jivesoftware.smack.roster.Roster$PresencePacketListener$1.run(Roster.java:1519) at org.jivesoftware.smack.AsyncButOrdered$Handler.run(AsyncButOrdered.java:121) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636) at java.lang.Thread.run(Thread.java:764) Thanks to Marcel Heckel for reporting this. Fixes SMACK-861. --- .../org/jivesoftware/smack/roster/Roster.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) 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 8e241151d..73486cd12 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 @@ -1471,7 +1471,29 @@ public final class Roster extends Manager { final Presence presence = (Presence) packet; final Jid from = presence.getFrom(); - final BareJid key = from != null ? from.asBareJid() : null; + final BareJid key; + if (from != null) { + key = from.asBareJid(); + } else { + XMPPConnection connection = connection(); + if (connection == null) { + LOGGER.finest("Connection was null while trying to handle exotic presence stanza: " + presence); + return; + } + // Assume the presence come "from the users account on the server" since no from was set (RFC 6120 ยง + // 8.1.2.1 4.). Note that getUser() may return null, but should never return null in this case as where + // connected. + EntityFullJid myJid = connection.getUser(); + if (myJid == null) { + LOGGER.info( + "Connection had no local address in Roster's presence listener." + + " Possibly we received a presence without from before being authenticated." + + " Presence: " + presence); + return; + } + LOGGER.info("Exotic presence stanza without from received: " + presence); + key = myJid.asBareJid(); + } asyncButOrdered.performAsyncButOrdered(key, new Runnable() { @Override