From e1bb5a6123087585814324084c017012c775245e Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 6 Jan 2013 14:02:44 +0000 Subject: [PATCH] SMACK-278: Call socket.close() in XMPPConnection.shutdown() before closing the reader and writer streams. This basically prevents the deadlock reported in SMACK-278. Additionally, we need to tell the PacketReader and PacketWriter that the socket got closed and that subsequent caused Exceptions can be ignored. This is done via the socketClosed boolean in XMPP Connection, that is queried by PacketReader and PacketWriter in case an Exception is encountered. git-svn-id: http://svn.igniterealtime.org/svn/repos/smack/trunk@13390 b35dd754-fafc-0310-a699-88a17e54d16e --- .../org/jivesoftware/smack/PacketReader.java | 7 ++- .../org/jivesoftware/smack/PacketWriter.java | 17 ++++++-- .../jivesoftware/smack/XMPPConnection.java | 43 +++++++++++++++++-- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/source/org/jivesoftware/smack/PacketReader.java b/source/org/jivesoftware/smack/PacketReader.java index f3c9b0350..616a18dbe 100644 --- a/source/org/jivesoftware/smack/PacketReader.java +++ b/source/org/jivesoftware/smack/PacketReader.java @@ -26,6 +26,7 @@ import org.jivesoftware.smack.sasl.SASLMechanism.Challenge; import org.jivesoftware.smack.sasl.SASLMechanism.Failure; import org.jivesoftware.smack.sasl.SASLMechanism.Success; import org.jivesoftware.smack.util.PacketParserUtils; + import org.xmlpull.mxp1.MXParser; import org.xmlpull.v1.XmlPullParser; import org.xmlpull.v1.XmlPullParserException; @@ -326,7 +327,9 @@ class PacketReader { } while (!done && eventType != XmlPullParser.END_DOCUMENT && thread == readerThread); } catch (Exception e) { - if (!done) { + // The exception can be ignored if the the connection is 'done' + // or if the it was caused because the socket got closed + if (!(done || connection.isSocketClosed())) { // Close the connection and notify connection listeners of the // error. notifyConnectionError(e); @@ -454,4 +457,4 @@ class PacketReader { } } } -} \ No newline at end of file +} diff --git a/source/org/jivesoftware/smack/PacketWriter.java b/source/org/jivesoftware/smack/PacketWriter.java index e845bdd07..8213bbd35 100644 --- a/source/org/jivesoftware/smack/PacketWriter.java +++ b/source/org/jivesoftware/smack/PacketWriter.java @@ -149,6 +149,9 @@ class PacketWriter { synchronized (queue) { queue.notifyAll(); } + // Interrupt the keep alive thread if one was created + if (keepAliveThread != null) + keepAliveThread.interrupt(); } /** @@ -232,10 +235,16 @@ class PacketWriter { } } } - catch (IOException ioe){ - if (!done) { + catch (IOException ioe) { + // The exception can be ignored if the the connection is 'done' + // or if the it was caused because the socket got closed + if (!(done || connection.isSocketClosed())) { done = true; - connection.packetReader.notifyConnectionError(ioe); + // packetReader could be set to null by an concurrent disconnect() call. + // Therefore Prevent NPE exceptions by checking packetReader. + if (connection.packetReader != null) { + connection.packetReader.notifyConnectionError(ioe); + } } } } @@ -307,4 +316,4 @@ class PacketWriter { } } } -} \ No newline at end of file +} diff --git a/source/org/jivesoftware/smack/XMPPConnection.java b/source/org/jivesoftware/smack/XMPPConnection.java index a81a26783..cd4f79e50 100644 --- a/source/org/jivesoftware/smack/XMPPConnection.java +++ b/source/org/jivesoftware/smack/XMPPConnection.java @@ -60,6 +60,10 @@ public class XMPPConnection extends Connection { String connectionID = null; private String user = null; private boolean connected = false; + // socketClosed is used concurrent + // by XMPPConnection, PacketReader, PacketWriter + private volatile boolean socketClosed = false; + /** * Flag that indicates if the user is currently authenticated with the server. */ @@ -358,6 +362,10 @@ public class XMPPConnection extends Connection { return isUsingTLS(); } + public boolean isSocketClosed() { + return socketClosed; + } + public boolean isAuthenticated() { return authenticated; } @@ -377,14 +385,20 @@ public class XMPPConnection extends Connection { */ protected void shutdown(Presence unavailablePresence) { // Set presence to offline. - packetWriter.sendPacket(unavailablePresence); + if (packetWriter != null) { + packetWriter.sendPacket(unavailablePresence); + } this.setWasAuthenticated(authenticated); authenticated = false; - connected = false; - packetReader.shutdown(); - packetWriter.shutdown(); + if (packetReader != null) { + packetReader.shutdown(); + } + if (packetWriter != null) { + packetWriter.shutdown(); + } + // Wait 150 ms for processes to clean-up, then shutdown. try { Thread.sleep(150); @@ -393,9 +407,25 @@ public class XMPPConnection extends Connection { // Ignore. } + // Set socketClosed to true. This will cause the PacketReader + // and PacketWriter to ingore any Exceptions that are thrown + // because of a read/write from/to a closed stream. + // It is *important* that this is done before socket.close()! + socketClosed = true; + try { + socket.close(); + } catch (Exception e) { + e.printStackTrace(); + } + // In most cases the close() should be successful, so set + // connected to false here. + connected = false; + // Close down the readers and writers. if (reader != null) { try { + // Should already be closed by the previous + // socket.close(). But just in case do it explicitly. reader.close(); } catch (Throwable ignore) { /* ignore */ } @@ -403,13 +433,17 @@ public class XMPPConnection extends Connection { } if (writer != null) { try { + // Should already be closed by the previous + // socket.close(). But just in case do it explicitly. writer.close(); } catch (Throwable ignore) { /* ignore */ } writer = null; } + // Make sure that the socket is really closed try { + // Does nothing if the socket is already closed socket.close(); } catch (Exception e) { @@ -524,6 +558,7 @@ public class XMPPConnection extends Connection { throw new XMPPException(errorMessage, new XMPPError( XMPPError.Condition.remote_server_error, errorMessage), ioe); } + socketClosed = false; initConnection(); }