From b28e8f43b97fdeb13a05a78d20b1d02194b96e3b Mon Sep 17 00:00:00 2001
From: Matthew Swift <matthew.swift@forgerock.com>
Date: Fri, 27 Sep 2013 22:20:48 +0000
Subject: [PATCH] Fix OPENDJ-1156 - NPE in ReferenceCountedObject after shutting down directory
---
opendj-sdk/opendj3/opendj-ldap-sdk/src/test/java/com/forgerock/opendj/ldap/ConnectionStateTest.java | 27 +++++++++++++++++++++++++++
opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/ConnectionState.java | 10 +++++++---
opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java | 13 ++++++-------
3 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/ConnectionState.java b/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/ConnectionState.java
index 5d4190a..9d1877d 100644
--- a/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/ConnectionState.java
+++ b/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/ConnectionState.java
@@ -98,10 +98,10 @@
@Override
boolean notifyConnectionClosed(final ConnectionState cs) {
+ cs.state = CLOSED;
for (final ConnectionEventListener listener : cs.listeners) {
listener.handleConnectionClosed();
}
- cs.state = CLOSED;
return true;
}
@@ -111,11 +111,15 @@
// Transition from valid to error state.
cs.failedDueToDisconnect = isDisconnectNotification;
cs.connectionError = error;
+ cs.state = ERROR;
+ /*
+ * FIXME: a re-entrant close will invoke close listeners before
+ * error notification has completed.
+ */
for (final ConnectionEventListener listener : cs.listeners) {
// Use the reason provided in the disconnect notification.
listener.handleConnectionError(isDisconnectNotification, error);
}
- cs.state = ERROR;
return true;
}
@@ -156,10 +160,10 @@
@Override
boolean notifyConnectionClosed(final ConnectionState cs) {
+ cs.state = ERROR_CLOSED;
for (final ConnectionEventListener listener : cs.listeners) {
listener.handleConnectionClosed();
}
- cs.state = ERROR_CLOSED;
return true;
}
},
diff --git a/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java b/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java
index 43f1bd0..3599b4f 100644
--- a/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java
+++ b/opendj-sdk/opendj3/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java
@@ -295,7 +295,6 @@
public void close(final UnbindRequest request, final String reason) {
// FIXME: I18N need to internationalize this message.
Validator.ensureNotNull(request);
-
close(request, false, Responses.newResult(ResultCode.CLIENT_SIDE_USER_CANCELLED)
.setDiagnosticMessage(reason != null ? reason : "Connection closed by client"));
}
@@ -624,10 +623,10 @@
}
/*
- * Now try cleanly closing the connection if possible. Only send unbind
- * if specified.
+ * If this is the final client initiated close then release close the
+ * connection and release resources.
*/
- if (unbindRequest != null) {
+ if (notifyClose) {
final ASN1BufferWriter asn1Writer = ASN1BufferWriter.getWriter();
try {
ldapWriter.unbindRequest(asn1Writer, nextMsgID.getAndIncrement(), unbindRequest);
@@ -640,10 +639,10 @@
} finally {
asn1Writer.recycle();
}
+ factory.getTimeoutChecker().removeConnection(this);
+ connection.closeSilently();
+ factory.releaseTransportAndTimeoutChecker();
}
- factory.getTimeoutChecker().removeConnection(this);
- connection.closeSilently();
- factory.releaseTransportAndTimeoutChecker();
// Notify listeners.
if (tmpListeners != null) {
diff --git a/opendj-sdk/opendj3/opendj-ldap-sdk/src/test/java/com/forgerock/opendj/ldap/ConnectionStateTest.java b/opendj-sdk/opendj3/opendj-ldap-sdk/src/test/java/com/forgerock/opendj/ldap/ConnectionStateTest.java
index 18ef073..aa0053a 100644
--- a/opendj-sdk/opendj3/opendj-ldap-sdk/src/test/java/com/forgerock/opendj/ldap/ConnectionStateTest.java
+++ b/opendj-sdk/opendj3/opendj-ldap-sdk/src/test/java/com/forgerock/opendj/ldap/ConnectionStateTest.java
@@ -29,6 +29,7 @@
import static org.forgerock.opendj.ldap.ErrorResultException.newErrorResult;
import static org.forgerock.opendj.ldap.responses.Responses.newGenericExtendedResult;
import static org.mockito.Matchers.same;
+import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -37,6 +38,8 @@
import org.forgerock.opendj.ldap.ErrorResultException;
import org.forgerock.opendj.ldap.ResultCode;
import org.forgerock.opendj.ldap.responses.ExtendedResult;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
import org.testng.annotations.Test;
/**
@@ -247,4 +250,28 @@
assertThat(state.isClosed()).isFalse();
assertThat(state.getConnectionError()).isNull();
}
+
+ /**
+ * Tests that reentrant close from error listener is handled.
+ */
+ @Test
+ public void testReentrantClose() {
+ final ConnectionState state = new ConnectionState();
+ final ConnectionEventListener listener1 = mock(ConnectionEventListener.class);
+ doAnswer(new Answer<Void>() {
+ public Void answer(InvocationOnMock invocation) {
+ state.notifyConnectionClosed();
+ return null;
+ }
+ }).when(listener1).handleConnectionError(false, ERROR);
+
+ state.addConnectionEventListener(listener1);
+ state.notifyConnectionError(false, ERROR);
+
+ assertThat(state.isValid()).isFalse();
+ assertThat(state.isClosed()).isTrue();
+ assertThat(state.getConnectionError()).isSameAs(ERROR);
+ verify(listener1).handleConnectionError(false, ERROR);
+ verify(listener1).handleConnectionClosed();
+ }
}
--
Gitblit v1.10.0