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,23 +921,46 @@ 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) {
// Listeners were already notified of the exception, return right here. ASYNC_BUT_ORDERED.performAsyncButOrdered(this, new Runnable() {
if (packetReader.done && packetWriter.done()) return; @Override
public void run() {
// Listeners were already notified of the exception, return right here.
if (packetReader.done || packetWriter.done()) return;
SmackWrappedException smackWrappedException = new SmackWrappedException(e); // Report the failure outside the synchronized block, so that a thread waiting within a synchronized
tlsHandled.reportGenericFailure(smackWrappedException); // function like connect() throws the wrapped exception.
saslFeatureReceived.reportGenericFailure(smackWrappedException); SmackWrappedException smackWrappedException = new SmackWrappedException(e);
maybeCompressFeaturesReceived.reportGenericFailure(smackWrappedException); tlsHandled.reportGenericFailure(smackWrappedException);
lastFeaturesReceived.reportGenericFailure(smackWrappedException); saslFeatureReceived.reportGenericFailure(smackWrappedException);
maybeCompressFeaturesReceived.reportGenericFailure(smackWrappedException);
lastFeaturesReceived.reportGenericFailure(smackWrappedException);
// Closes the connection temporary. A reconnection is possible synchronized (XMPPTCPConnection.this) {
// Note that a connection listener of XMPPTCPConnection will drop the SM state in // Within this synchronized block, either *both* reader and writer threads must be terminated, or
// case the Exception is a StreamErrorException. // none.
instantShutdown(); assert ((packetReader.done && packetWriter.done())
|| (!packetReader.done && !packetWriter.done()));
// Notify connection listeners of the error. // Closes the connection temporary. A reconnection is possible
callConnectionClosedOnErrorListener(e); // Note that a connection listener of XMPPTCPConnection will drop the SM state in
// case the Exception is a StreamErrorException.
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.
callConnectionClosedOnErrorListener(e);
}
}, XMPPTCPConnection.this + " callConnectionClosedOnErrorListener()");
}
});
} }
/** /**
@ -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()");
disconnect(); ASYNC_BUT_ORDERED.performAsyncButOrdered(XMPPTCPConnection.this, new Runnable() {
@Override
public void run() {
disconnect();
}});
} }
} }
break; break;