Do not call synchronized methods in reader/writer thread

This may cause deadlocks with a call to acquire(2) on the introduced
readerWriterSemaphore in initConnection(), which is also synchronized.
This commit is contained in:
Florian Schmaus 2019-03-06 22:11:45 +01:00
parent 3d1a781a22
commit 7d2c3ac9f9
2 changed files with 43 additions and 16 deletions

View File

@ -297,7 +297,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection {
} }
}); });
private static final AsyncButOrdered<AbstractXMPPConnection> ASYNC_BUT_ORDERED = new AsyncButOrdered<>(); protected static final AsyncButOrdered<AbstractXMPPConnection> ASYNC_BUT_ORDERED = new AsyncButOrdered<>();
/** /**
* The used host to establish the connection to * The used host to establish the connection to

View File

@ -921,24 +921,47 @@ public class XMPPTCPConnection extends AbstractXMPPConnection {
* *
* @param e the exception that causes the connection close event. * @param e the exception that causes the connection close event.
*/ */
private synchronized void notifyConnectionError(Exception e) { private synchronized void notifyConnectionError(final Exception e) {
ASYNC_BUT_ORDERED.performAsyncButOrdered(this, new Runnable() {
@Override
public void run() {
// Listeners were already notified of the exception, return right here. // Listeners were already notified of the exception, return right here.
if (packetReader.done && packetWriter.done()) return; if (packetReader.done || packetWriter.done()) return;
// Report the failure outside the synchronized block, so that a thread waiting within a synchronized
// function like connect() throws the wrapped exception.
SmackWrappedException smackWrappedException = new SmackWrappedException(e); SmackWrappedException smackWrappedException = new SmackWrappedException(e);
tlsHandled.reportGenericFailure(smackWrappedException); tlsHandled.reportGenericFailure(smackWrappedException);
saslFeatureReceived.reportGenericFailure(smackWrappedException); saslFeatureReceived.reportGenericFailure(smackWrappedException);
maybeCompressFeaturesReceived.reportGenericFailure(smackWrappedException); maybeCompressFeaturesReceived.reportGenericFailure(smackWrappedException);
lastFeaturesReceived.reportGenericFailure(smackWrappedException); lastFeaturesReceived.reportGenericFailure(smackWrappedException);
synchronized (XMPPTCPConnection.this) {
// Within this synchronized block, either *both* reader and writer threads must be terminated, or
// none.
assert ((packetReader.done && packetWriter.done())
|| (!packetReader.done && !packetWriter.done()));
// Closes the connection temporary. A reconnection is possible // Closes the connection temporary. A reconnection is possible
// Note that a connection listener of XMPPTCPConnection will drop the SM state in // Note that a connection listener of XMPPTCPConnection will drop the SM state in
// case the Exception is a StreamErrorException. // case the Exception is a StreamErrorException.
instantShutdown(); instantShutdown();
// Wait for reader and writer threads to be terminated.
readerWriterSemaphore.acquireUninterruptibly(2);
readerWriterSemaphore.release(2);
}
Async.go(new Runnable() {
@Override
public void run() {
// Notify connection listeners of the error. // Notify connection listeners of the error.
callConnectionClosedOnErrorListener(e); callConnectionClosedOnErrorListener(e);
} }
}, XMPPTCPConnection.this + " callConnectionClosedOnErrorListener()");
}
});
}
/** /**
* For unit testing purposes * For unit testing purposes
@ -1248,7 +1271,11 @@ public class XMPPTCPConnection extends AbstractXMPPConnection {
LOGGER.info(XMPPTCPConnection.this LOGGER.info(XMPPTCPConnection.this
+ " received closing </stream> element." + " received closing </stream> element."
+ " Server wants to terminate the connection, calling disconnect()"); + " Server wants to terminate the connection, calling disconnect()");
ASYNC_BUT_ORDERED.performAsyncButOrdered(XMPPTCPConnection.this, new Runnable() {
@Override
public void run() {
disconnect(); disconnect();
}});
} }
} }
break; break;