1
0
Fork 0
mirror of https://github.com/vanitasvitae/Smack.git synced 2024-11-22 03:52:06 +01:00

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]: <iq id='qn03S-26' type='get'>
Jun 11 23:11:11 c2s18c2370      debug   Received[c2s]: <presence id='qn03S-27'>
Jun 11 23:11:11 c2s18c2370      debug   Received[c2s]: <presence id='qn03S-23' type='unavailable'>
Jun 11 23:11:11 c2s18c2370      debug   Received </stream:stream>

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.
This commit is contained in:
Florian Schmaus 2015-06-13 16:58:57 +02:00
parent 989076a166
commit c9eb6323c0

View file

@ -465,6 +465,16 @@ public class XMPPTCPConnection extends AbstractXMPPConnection {
packetWriter.shutdown(instant); 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) { if (packetReader != null) {
packetReader.shutdown(); packetReader.shutdown();
} }
@ -1101,8 +1111,15 @@ public class XMPPTCPConnection extends AbstractXMPPConnection {
LOGGER.warning(XMPPTCPConnection.this + " </stream> but different namespace " + parser.getNamespace()); LOGGER.warning(XMPPTCPConnection.this + " </stream> but different namespace " + parser.getNamespace());
break; 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(); closingStreamReceived.reportSuccess();
if (packetWriter.queue.isShutdown()) {
if (queueWasShutdown) {
// We received a closing stream element *after* we initiated the // We received a closing stream element *after* we initiated the
// termination of the session by sending a closing stream element to // termination of the session by sending a closing stream element to
// the server first // the server first
@ -1382,17 +1399,6 @@ public class XMPPTCPConnection extends AbstractXMPPConnection {
LOGGER.log(Level.WARNING, "Exception writing closing stream element", e); 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). // Delete the queue contents (hopefully nothing is left).
queue.clear(); queue.clear();
} else if (instantShutdown && isSmEnabled()) { } else if (instantShutdown && isSmEnabled()) {
@ -1400,14 +1406,10 @@ public class XMPPTCPConnection extends AbstractXMPPConnection {
// into the unacknowledgedStanzas queue // into the unacknowledgedStanzas queue
drainWriterQueueToUnacknowledgedStanzas(); drainWriterQueueToUnacknowledgedStanzas();
} }
// Do *not* close the writer here, as it will cause the socket
try { // to get closed. But we may want to receive further stanzas
writer.close(); // until the closing stream tag is received. The socket will be
} // closed in shutdown().
catch (Exception e) {
// Do nothing
}
} }
catch (Exception e) { catch (Exception e) {
// The exception can be ignored if the the connection is 'done' // The exception can be ignored if the the connection is 'done'