From 976da4aa6126aefd775b7c223c71e29d1207bf10 Mon Sep 17 00:00:00 2001
From: Matthew Swift <matthew.swift@forgerock.com>
Date: Wed, 11 Dec 2013 17:17:53 +0000
Subject: [PATCH] Fix OPENDJ-1247: Client side timeouts do not cancel bind or startTLS requests properly
---
opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java | 133 +++++++++++++++++++++++++++++++++----------
1 files changed, 101 insertions(+), 32 deletions(-)
diff --git a/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java b/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java
index 68c95ed..8829372 100644
--- a/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java
+++ b/opendj-ldap-sdk/src/main/java/com/forgerock/opendj/ldap/LDAPConnection.java
@@ -28,7 +28,9 @@
package com.forgerock.opendj.ldap;
import static com.forgerock.opendj.util.StaticUtils.DEBUG_LOG;
+import static org.forgerock.opendj.ldap.CoreMessages.*;
import static org.forgerock.opendj.ldap.ErrorResultException.newErrorResult;
+import static org.forgerock.opendj.ldap.requests.Requests.newAbandonRequest;
import java.io.IOException;
import java.net.InetSocketAddress;
@@ -65,7 +67,6 @@
import org.forgerock.opendj.ldap.requests.GenericBindRequest;
import org.forgerock.opendj.ldap.requests.ModifyDNRequest;
import org.forgerock.opendj.ldap.requests.ModifyRequest;
-import org.forgerock.opendj.ldap.requests.Requests;
import org.forgerock.opendj.ldap.requests.SearchRequest;
import org.forgerock.opendj.ldap.requests.StartTLSExtendedRequest;
import org.forgerock.opendj.ldap.requests.UnbindRequest;
@@ -128,40 +129,64 @@
@Override
public FutureResult<Void> abandonAsync(final AbandonRequest request) {
- final AbstractLDAPFutureResultImpl<?> pendingRequest;
- final int messageID = nextMsgID.getAndIncrement();
+ /*
+ * Need to be careful here since both abandonAsync and Future.cancel can
+ * be called separately by the client application. Therefore
+ * future.cancel() should abandon the request, and abandonAsync should
+ * cancel the future. In addition, bind or StartTLS requests cannot be
+ * abandoned.
+ */
try {
synchronized (stateLock) {
checkConnectionIsValid();
- checkBindOrStartTLSInProgress();
- // Remove the future associated with the request to be abandoned.
- pendingRequest = pendingRequests.remove(request.getRequestID());
- }
- if (pendingRequest == null) {
/*
- * There has never been a request with the specified message ID
- * or the response has already been received and handled. We can
- * ignore this abandon request.
+ * If there is a bind or startTLS in progress then it must be
+ * this request which is being abandoned. The following check
+ * will prevent it from happening.
*/
-
- // Message ID will be -1 since no request was sent.
- return new CompletedFutureResult<Void>((Void) null);
- }
- pendingRequest.cancel(false);
- try {
- final ASN1BufferWriter asn1Writer = ASN1BufferWriter.getWriter();
- try {
- ldapWriter.abandonRequest(asn1Writer, messageID, request);
- connection.write(asn1Writer.getBuffer(), null);
- return new CompletedFutureResult<Void>((Void) null, messageID);
- } finally {
- asn1Writer.recycle();
- }
- } catch (final IOException e) {
- throw adaptRequestIOException(e);
+ checkBindOrStartTLSInProgress();
}
} catch (final ErrorResultException e) {
- return new CompletedFutureResult<Void>(e, messageID);
+ return new CompletedFutureResult<Void>(e);
+ }
+
+ // Remove the future associated with the request to be abandoned.
+ final AbstractLDAPFutureResultImpl<?> pendingRequest =
+ pendingRequests.remove(request.getRequestID());
+ if (pendingRequest == null) {
+ /*
+ * There has never been a request with the specified message ID or
+ * the response has already been received and handled. We can ignore
+ * this abandon request.
+ */
+ return new CompletedFutureResult<Void>((Void) null);
+ }
+
+ /*
+ * This will cancel the future, but will also recursively invoke this
+ * method. Since the pending request has been removed, there is no risk
+ * of an infinite loop.
+ */
+ pendingRequest.cancel(false);
+
+ /*
+ * FIXME: there's a potential race condition here if a bind or startTLS
+ * is initiated just after we removed the pending request.
+ */
+ return sendAbandonRequest(request);
+ }
+
+ private FutureResult<Void> sendAbandonRequest(final AbandonRequest request) {
+ final ASN1BufferWriter asn1Writer = ASN1BufferWriter.getWriter();
+ try {
+ final int messageID = nextMsgID.getAndIncrement();
+ ldapWriter.abandonRequest(asn1Writer, messageID, request);
+ connection.write(asn1Writer.getBuffer(), null);
+ return new CompletedFutureResult<Void>((Void) null, messageID);
+ } catch (final IOException e) {
+ return new CompletedFutureResult<Void>(adaptRequestIOException(e));
+ } finally {
+ asn1Writer.recycle();
}
}
@@ -557,10 +582,54 @@
if (future != null && future.checkForTimeout()) {
final long diff = (future.getTimestamp() + timeout) - currentTime;
if (diff <= 0 && pendingRequests.remove(requestID) != null) {
- DEBUG_LOG.fine("Cancelling expired future result: " + future);
- final Result result = Responses.newResult(ResultCode.CLIENT_SIDE_TIMEOUT);
- future.adaptErrorResult(result);
- abandonAsync(Requests.newAbandonRequest(future.getRequestID()));
+ if (future.isBindOrStartTLS()) {
+ /*
+ * No other operations can be performed while a bind
+ * or StartTLS request is active, so we cannot time
+ * out the request. We therefore have a choice:
+ * either ignore timeouts for these operations, or
+ * enforce them but doing so requires invalidating
+ * the connection. We'll do the latter, since
+ * ignoring timeouts could cause the application to
+ * hang.
+ */
+ DEBUG_LOG.fine("Failing bind or StartTLS request due to timeout "
+ + "(connection will be invalidated): " + future);
+ final Result result =
+ Responses.newResult(ResultCode.CLIENT_SIDE_TIMEOUT)
+ .setDiagnosticMessage(
+ LDAP_CONNECTION_BIND_OR_START_TLS_REQUEST_TIMEOUT
+ .get(timeout).toString());
+ future.adaptErrorResult(result);
+
+ // Fail the connection.
+ final Result errorResult =
+ Responses.newResult(ResultCode.CLIENT_SIDE_TIMEOUT)
+ .setDiagnosticMessage(
+ LDAP_CONNECTION_BIND_OR_START_TLS_CONNECTION_TIMEOUT
+ .get(timeout).toString());
+ connectionErrorOccurred(errorResult);
+ } else {
+ DEBUG_LOG.fine("Failing request due to timeout: " + future);
+ final Result result =
+ Responses.newResult(ResultCode.CLIENT_SIDE_TIMEOUT)
+ .setDiagnosticMessage(
+ LDAP_CONNECTION_REQUEST_TIMEOUT.get(timeout)
+ .toString());
+ future.adaptErrorResult(result);
+
+ /*
+ * FIXME: there's a potential race condition here if
+ * a bind or startTLS is initiated just after we
+ * check the boolean. It seems potentially even more
+ * dangerous to send the abandon request while
+ * holding the state lock, since a blocking write
+ * could hang the application.
+ */
+ if (!bindOrStartTLSInProgress.get()) {
+ sendAbandonRequest(newAbandonRequest(requestID));
+ }
+ }
} else {
delay = Math.min(delay, diff);
}
--
Gitblit v1.10.0