From c9eb6323c0d8c739e378815de120028db3c8d297 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sat, 13 Jun 2015 16:58:57 +0200 Subject: [PATCH] Fix a race on graceful disconnect A sequence of connect(), disconnect(), and connect() could cause the connection to get disconnected again by the old reader thread, which was blocking on disconnect because a closing stream tag was read. If the second connect() was processed before the disconnect(), then the connectin would get disconnected right after the second connect(). This showed up as a "strange" sequence of stanzas in the XMPP servers log. Note that the stanza ID of the unavailable presence has a lower number then the previous stanzas: Jun 11 23:11:11 c2s18c2370 debug Resource bound: smack-inttest-two-93t70@geekplace.eu/two-93t70 Jun 11 23:11:11 c2s18c2370 debug Received[c2s]: Jun 11 23:11:11 c2s18c2370 debug Received[c2s]: Jun 11 23:11:11 c2s18c2370 debug Received[c2s]: Jun 11 23:11:11 c2s18c2370 debug Received This is because the disconnect() of the first reader thread could generate the unavailable presence, but was blocked afterwards when entering the synchronized disconnect(Presence unavailablePresence). SMACK-633. --- .../smack/tcp/XMPPTCPConnection.java | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 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 969be7033..10f24350b 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 @@ -465,6 +465,16 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { packetWriter.shutdown(instant); } + try { + // After we send the closing stream element, check if there was already a + // closing stream element sent by the server or wait with a timeout for a + // closing stream element to be received from the server. + Exception res = closingStreamReceived.checkIfSuccessOrWait(); + LOGGER.info("closingstream " + res); + } catch (InterruptedException | NoResponseException e) { + LOGGER.log(Level.INFO, "Exception while waiting for closing stream element from the server " + this, e); + } + if (packetReader != null) { packetReader.shutdown(); } @@ -1101,8 +1111,15 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { LOGGER.warning(XMPPTCPConnection.this + " but different namespace " + parser.getNamespace()); break; } + + // Check if the queue was already shut down before reporting success on closing stream tag + // received. This avoids a race if there is a disconnect(), followed by a connect(), which + // did re-start the queue again, causing this writer to assume that the queue is not + // shutdown, which results in a call to disconnect(). + final boolean queueWasShutdown = packetWriter.queue.isShutdown(); closingStreamReceived.reportSuccess(); - if (packetWriter.queue.isShutdown()) { + + if (queueWasShutdown) { // We received a closing stream element *after* we initiated the // termination of the session by sending a closing stream element to // the server first @@ -1382,17 +1399,6 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { LOGGER.log(Level.WARNING, "Exception writing closing stream element", e); } - try { - // After we send the closing stream element, check if there was already a - // closing stream element sent by the server or wait with a timeout for a - // closing stream element to be received from the server. - closingStreamReceived.checkIfSuccessOrWait(); - } catch (NoResponseException e) { - LOGGER.log(Level.INFO, "No response while waiting for closing stream element from the server (" + getConnectionCounter() + ")", e); - } catch (InterruptedException e) { - LOGGER.log(Level.INFO, "Waiting for closing stream element from the server was interrupted (" + getConnectionCounter() + ")", e); - } - // Delete the queue contents (hopefully nothing is left). queue.clear(); } else if (instantShutdown && isSmEnabled()) { @@ -1400,14 +1406,10 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { // into the unacknowledgedStanzas queue drainWriterQueueToUnacknowledgedStanzas(); } - - try { - writer.close(); - } - catch (Exception e) { - // Do nothing - } - + // Do *not* close the writer here, as it will cause the socket + // to get closed. But we may want to receive further stanzas + // until the closing stream tag is received. The socket will be + // closed in shutdown(). } catch (Exception e) { // The exception can be ignored if the the connection is 'done'