You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@directory.apache.org by se...@apache.org on 2019/05/26 20:53:25 UTC
[directory-ldap-api] branch master updated: DIRAPI-342:
Unbind/close breaks connection
This is an automated email from the ASF dual-hosted git repository.
seelmann pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/directory-ldap-api.git
The following commit(s) were added to refs/heads/master by this push:
new 125889b DIRAPI-342: Unbind/close breaks connection
125889b is described below
commit 125889b6e094be7659f9f448027e79029dcfa890
Author: Stefan Seelmann <ma...@stefan-seelmann.de>
AuthorDate: Sun May 26 22:52:44 2019 +0200
DIRAPI-342: Unbind/close breaks connection
* Fix race condition in `sessionClosed()` callback: make synchronized and check if same session is closed
* Fix race condition in `setCloseListener()` callback: make synchronized and check if same session is closed
* Move duplicated code from `unbind()` and `sessionClosed()` to `close()`
* Remove `connected` flag, use information from session instead
* Remove no longer required lock
* Reuse isConnected() method where possible
---
.../ldap/client/api/LdapNetworkConnection.java | 213 +++++++++------------
1 file changed, 94 insertions(+), 119 deletions(-)
diff --git a/ldap/client/api/src/main/java/org/apache/directory/ldap/client/api/LdapNetworkConnection.java b/ldap/client/api/src/main/java/org/apache/directory/ldap/client/api/LdapNetworkConnection.java
index 7c86a16..b10a887 100644
--- a/ldap/client/api/src/main/java/org/apache/directory/ldap/client/api/LdapNetworkConnection.java
+++ b/ldap/client/api/src/main/java/org/apache/directory/ldap/client/api/LdapNetworkConnection.java
@@ -42,7 +42,6 @@ import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.locks.ReentrantLock;
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManager;
@@ -196,7 +195,7 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
private long timeout = LdapConnectionConfig.DEFAULT_TIMEOUT;
/** configuration object for the connection */
- private LdapConnectionConfig config;
+ private final LdapConnectionConfig config;
/** The Socket configuration */
private SocketSessionConfig socketSessionConfig;
@@ -204,9 +203,6 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
/** The connector open with the remote server */
private IoConnector connector;
- /** A mutex used to avoid a double close of the connector */
- private ReentrantLock connectorMutex = new ReentrantLock();
-
/**
* The created session, created when we open a connection with
* the Ldap server.
@@ -214,7 +210,7 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
private IoSession ldapSession;
/** a map to hold the ResponseFutures for all operations */
- private Map<Integer, ResponseFuture<? extends Response>> futureMap = new ConcurrentHashMap<>();
+ private final Map<Integer, ResponseFuture<? extends Response>> futureMap = new ConcurrentHashMap<>();
/** list of controls supported by the server */
private List<String> supportedControls;
@@ -225,16 +221,13 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
/** A flag indicating that the BindRequest has been issued and successfully authenticated the user */
private AtomicBoolean authenticated = new AtomicBoolean( false );
- /** A flag indicating that the connection is connected or not */
- private AtomicBoolean connected = new AtomicBoolean( false );
-
/** a list of listeners interested in getting notified when the
* connection's session gets closed cause of network issues
*/
private List<ConnectionClosedEventListener> conCloseListeners;
/** The Ldap codec protocol filter */
- private IoFilter ldapProtocolFilter = new ProtocolCodecFilter( codec.getProtocolCodecFactory() );
+ private final IoFilter ldapProtocolFilter = new ProtocolCodecFilter( codec.getProtocolCodecFactory() );
/** the SslFilter key */
private static final String SSL_FILTER_KEY = "sslFilter";
@@ -244,10 +237,10 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
/** The krb5 configuration property */
private static final String KRB5_CONF = "java.security.krb5.conf";
-
- /** A future used to block any action until the handhake is completed */
+
+ /** A future used to block any action until the handshake is completed */
private HandshakeFuture handshakeFuture;
-
+
// ~~~~~~~~~~~~~~~~~ common error messages ~~~~~~~~~~~~~~~~~~~~~~~~~~
static final String TIME_OUT_ERROR = I18n.err( I18n.ERR_04170_TIMEOUT_OCCURED );
@@ -531,7 +524,7 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
@Override
public boolean isConnected()
{
- return ( ldapSession != null ) && connected.get() && !ldapSession.isClosing();
+ return ( ldapSession != null ) && ldapSession.isConnected() && !ldapSession.isClosing();
}
@@ -569,7 +562,7 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
throw new InvalidConnectionException( I18n.err( I18n.ERR_04104_NULL_CONNECTION_CANNOT_CONNECT ) );
}
- if ( !connected.get() )
+ if ( !isConnected() )
{
throw new InvalidConnectionException( I18n.err( I18n.ERR_04108_INVALID_CONNECTION ) );
}
@@ -824,65 +817,74 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
closeFuture.addListener( future ->
{
- // Process all the waiting operations and cancel them
- if ( LOG.isDebugEnabled() )
+ synchronized ( LdapNetworkConnection.this )
{
- LOG.debug( I18n.msg( I18n.MSG_04137_NOD_RECEIVED ) );
- }
+ // DIRAPI-342: Skip processing if LDAP session changed, i.e. after re-connect.
+ if ( future.getSession() != ldapSession )
+ {
+ return;
+ }
- for ( ResponseFuture<?> responseFuture : futureMap.values() )
- {
+ // Process all the waiting operations and cancel them
if ( LOG.isDebugEnabled() )
{
- LOG.debug( I18n.msg( I18n.MSG_04134_CLOSING, responseFuture ) );
+ LOG.debug( I18n.msg( I18n.MSG_04137_NOD_RECEIVED ) );
}
- responseFuture.cancel();
-
- try
+ for ( ResponseFuture<?> responseFuture : futureMap.values() )
{
- if ( responseFuture instanceof AddFuture )
+ if ( LOG.isDebugEnabled() )
{
- ( ( AddFuture ) responseFuture ).set( AddNoDResponse.PROTOCOLERROR );
+ LOG.debug( I18n.msg( I18n.MSG_04134_CLOSING, responseFuture ) );
}
- else if ( responseFuture instanceof BindFuture )
- {
- ( ( BindFuture ) responseFuture ).set( BindNoDResponse.PROTOCOLERROR );
- }
- else if ( responseFuture instanceof CompareFuture )
- {
- ( ( CompareFuture ) responseFuture ).set( CompareNoDResponse.PROTOCOLERROR );
- }
- else if ( responseFuture instanceof DeleteFuture )
- {
- ( ( DeleteFuture ) responseFuture ).set( DeleteNoDResponse.PROTOCOLERROR );
- }
- else if ( responseFuture instanceof ExtendedFuture )
- {
- ( ( ExtendedFuture ) responseFuture ).set( ExtendedNoDResponse.PROTOCOLERROR );
- }
- else if ( responseFuture instanceof ModifyFuture )
- {
- ( ( ModifyFuture ) responseFuture ).set( ModifyNoDResponse.PROTOCOLERROR );
- }
- else if ( responseFuture instanceof ModifyDnFuture )
+
+ responseFuture.cancel();
+
+ try
{
- ( ( ModifyDnFuture ) responseFuture ).set( ModifyDnNoDResponse.PROTOCOLERROR );
+ if ( responseFuture instanceof AddFuture )
+ {
+ ( ( AddFuture ) responseFuture ).set( AddNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof BindFuture )
+ {
+ ( ( BindFuture ) responseFuture ).set( BindNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof CompareFuture )
+ {
+ ( ( CompareFuture ) responseFuture ).set( CompareNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof DeleteFuture )
+ {
+ ( ( DeleteFuture ) responseFuture ).set( DeleteNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof ExtendedFuture )
+ {
+ ( ( ExtendedFuture ) responseFuture ).set( ExtendedNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof ModifyFuture )
+ {
+ ( ( ModifyFuture ) responseFuture ).set( ModifyNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof ModifyDnFuture )
+ {
+ ( ( ModifyDnFuture ) responseFuture ).set( ModifyDnNoDResponse.PROTOCOLERROR );
+ }
+ else if ( responseFuture instanceof SearchFuture )
+ {
+ ( ( SearchFuture ) responseFuture ).set( SearchNoDResponse.PROTOCOLERROR );
+ }
}
- else if ( responseFuture instanceof SearchFuture )
+ catch ( InterruptedException e )
{
- ( ( SearchFuture ) responseFuture ).set( SearchNoDResponse.PROTOCOLERROR );
+ LOG.error( I18n.err( I18n.ERR_04113_ERROR_PROCESSING_NOD, responseFuture ), e );
}
- }
- catch ( InterruptedException e )
- {
- LOG.error( I18n.err( I18n.ERR_04113_ERROR_PROCESSING_NOD, responseFuture ), e );
+
+ futureMap.remove( messageId.get() );
}
- futureMap.remove( messageId.get() );
+ futureMap.clear();
}
-
- futureMap.clear();
} );
}
@@ -926,7 +928,7 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
@Override
public boolean connect() throws LdapException
{
- if ( ( ldapSession != null ) && connected.get() )
+ if ( isConnected() )
{
// No need to connect if we already have a connected session
return true;
@@ -974,34 +976,43 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
* {@inheritDoc}
*/
@Override
- public void close()
+ public synchronized void close()
{
- // Close the session
- if ( ( ldapSession != null ) && connected.get() )
+ if ( !isConnected() )
{
- ldapSession.closeNow();
- connected.set( false );
+ return;
}
+ // Close the session
+ ldapSession.closeNow().awaitUninterruptibly( timeout );
+ ldapSession = null;
+
+ clearMaps();
+
// And close the connector if it has been created locally
// Release the connector
- connectorMutex.lock();
- try
- {
- if ( connector != null )
- {
- connector.dispose();
- connector = null;
- }
- }
- finally
+ if ( connector != null )
{
- connectorMutex.unlock();
+ connector.dispose();
+ connector = null;
}
// Reset the messageId
messageId.set( 0 );
+
+ if ( conCloseListeners != null )
+ {
+ if ( LOG.isDebugEnabled() )
+ {
+ LOG.debug( I18n.msg( I18n.MSG_04136_NOTIFYING_CLOSE_LISTENERS ) );
+ }
+
+ for ( ConnectionClosedEventListener listener : conCloseListeners )
+ {
+ listener.connectionClosed();
+ }
+ }
}
@@ -2394,17 +2405,9 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
responseFuture.cancel();
}
- // clear the mappings
- clearMaps();
-
// We now have to close the session
close();
- connected.set( false );
-
- // Last, not least, reset the MessageId value
- messageId.set( 0 );
-
// And get out
if ( LOG.isDebugEnabled() )
{
@@ -4746,7 +4749,6 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
codec, config.getBinaryAttributeDetector() );
session.setAttribute( LdapDecoder.MESSAGE_CONTAINER_ATTR, ldapMessageContainer );
- connected.set( true );
}
@@ -4754,48 +4756,21 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
* {@inheritDoc}
*/
@Override
- public void sessionClosed( IoSession session ) throws Exception
+ public synchronized void sessionClosed( IoSession session ) throws Exception
{
- // no need to handle if this session was closed by the user
- if ( ldapSession == null || !connected.get() )
+ // DIRAPI-342: Skip processing if LDAP session changed, i.e. after re-connect.
+ if ( session != ldapSession )
{
return;
}
- ldapSession.closeNow();
- connected.set( false );
- // Reset the messageId
- messageId.set( 0 );
-
- connectorMutex.lock();
-
- try
- {
- if ( connector != null )
- {
- connector.dispose();
- connector = null;
- }
- }
- finally
+ // no need to handle if this session was closed by the user
+ if ( !isConnected() )
{
- connectorMutex.unlock();
+ return;
}
- clearMaps();
-
- if ( conCloseListeners != null )
- {
- if ( LOG.isDebugEnabled() )
- {
- LOG.debug( I18n.msg( I18n.MSG_04136_NOTIFYING_CLOSE_LISTENERS ) );
- }
-
- for ( ConnectionClosedEventListener listener : conCloseListeners )
- {
- listener.connectionClosed();
- }
- }
+ close();
}
@@ -4901,7 +4876,7 @@ public class LdapNetworkConnection extends AbstractLdapConnection implements Lda
// for LDAPS/TLS
handshakeFuture = new HandshakeFuture();
- if ( ( ldapSession == null ) || !connected.get() )
+ if ( !isConnected() )
{
connector.getFilterChain().addFirst( SSL_FILTER_KEY, sslFilter );
}