pingidentity
LDAP SDK and InterruptedException handling
Hi there,
Thx for the API, I would say is really great, and the best Java LDAP API I have worked with.
My issue is related with usage of the API in a multi threaded environment. I am using LDAPConnectionPool and sharing between multiple threads. Everything works fine, but I have found a little strange the behaviour of the API when the threads are interrupted. In some cases I got this errors:
LDAPSearchException(resultCode=82 (local error), numEntries=0, numReferences=0, errorMessage='Search processing was interrupted while waiting for a response from server localhost:6000.') at com.unboundid.ldap.sdk.LDAPConnection.search(LDAPConnection.java:3645) at com.unboundid.ldap.sdk.AbstractConnectionPool.search(AbstractConnectionPool.java:2022) at com.unboundid.ldap.sdk.AbstractConnectionPool.search(AbstractConnectionPool.java:1759)
When a thraed using the API is interrupted and that is mapped to error code 82; but error 82 is also used also for internal errors. That makes very tricky to properly handle the interrupt signal in threads using the API (unless you parse the error message, that I think it is bad practice). In my view, it could much cleaner that the API is able to throw InterruptedException when the thread is interrupted while waiting for LDAP server response.
What do you think? What is the best approach with the current API to properly handle interrupt signal in threads using the SDK?
Thanks in advance,
/Evaristo
For better or worse, LDAP result codes aren't meant to provide a one-to-one mapping to uniquely identify every possible condition. There are a number of well-defined standard result codes that cover most cases, and some directory servers may define custom result codes for specific purposes that might need to be handled specially.
The LDAP SDK doesn't directly catch InterruptedException in the course of reading from the server, but does catch Exception as a way of covering all its bases for unexpected client-side conditions, and the InterruptedException is getting caught up in that. I'm not sure that catching and re-throwing InterruptedException is the best approach, since the caller isn't likely to be prepared for that condition, and the way that most clients will handle it is generally the same as for any other unexpected condition that prevents the SDK from getting an expected response.
However, if an LDAPException is triggered by some other exception, then that exception should be available as the cause. If you want to do something special for an InterruptedException, you can check the cause and take the appropriate action. Note that it's possible that the exception could be caught higher up and wrapped in another exception (for example, in this case, it looks like an LDAPException is being caught and re-thrown as an LDAPSearchException), so you might need to check to see if the cause is an LDAPException and if so, then check its cause (and possibly recursively).
Hi there,
Thx for the fast reply, but let me explain a bit furthermy point.
Any multi-threaded application needs to handle the Interrupt signal in a proper way. Interrupted exception is not something unexpected for the LDAP client application; it is a design choice to use threads to build a perfpormant LDAP client app and if that is the case there should a proper way to handle the threads life cycle. If one thread is blocked waiting for a search response from an LDAP server and the threads are interrupted (imagine a propoer shutdown for the app is requested), in this case the SDK returns an error code (82), that makes almost impossible for the SDK user to properly terminate the thread. The only way is to explicitly check for error 82 and check the error message; I think it is much cleaner to throw the Interrupted exception that is really the condition that made the SDK to throw an error.
I was checking the source code, of the SDK and for instance SearchRequest.java handles Interrupted internally in this way: catch (InterruptedException ie) { debugException(ie); throw new LDAPException(ResultCode.LOCAL_ERROR, ERR_SEARCH_INTERRUPTED.get(connection.getHostPort()), ie); }
That means, that the SDK consumed the InterruptedException and the interrupt signal from the thread is removed, so in my view the SDK should at least set the interruot signal like this:
catch (InterruptedException ie)
{
Thread.currentThread().interrupt()
debugException(ie);
throw new LDAPException(ResultCode.LOCAL_ERROR,
ERR_SEARCH_INTERRUPTED.get(connection.getHostPort()), ie);
}
At least this will allow the app using the SDK terminate threads properly. Otherwise, there is no way for the caller app to know that their threads has been requested to finished properly.
regards,
/evaristo
The LDAP SDK can't throw an InterruptedException, because that would break backward compatibility, which is something that we won't do.
I also disagree with the assertion that all multithreaded applications need to handle interrupt signals. Interrupting a thread that isn't explicitly known to handle interrupts properly is potentially very dangerous. For example, if you try to interrupt any thread interacting with the Berkeley DB Java Edition, you will make the database extremely angry and refuse to honor any subsequent requests until you close everything down and re-open it.
I'm also not sure that catching and re-setting the interrupt flag on the thread is a great idea. It may work in this specific case, but there are other cases in the SDK in which it may not explicitly catch InterruptedException, but lump it in as part of a broader catch. However, in the event that the LDAP SDK catches an exception that triggers an LDAPException, it will always include the caught exception in the cause of the LDAPException. If you have a specific reason to look for InterruptedException, you can do so with code like:
public boolean isInterrupted(LDAPException le)
{
Throwable cause = le.getCause();
while (cause != null)
{
if (cause instanceof InterruptedException)
{
return true;
}
cause = cause.getCause();
}
return false;
}
You shouldn't use InterruptedException as a signal that a thread should shut down because there can be other reasons that the interrupted flag might be set. It's better to have a flag in your application that you can check to see whether it should shut down. And if you want to interrupt a thread that might be in the middle of processing an operation, the best thing to do is close the connection or use an LDAP cancel extended operation. Of course, you can also process operations using the asynchronous API, which provides more flexibility in handling operations beyond simply blocking while waiting for a response.
And I don't understand why you say that using result code 82 makes it almost impossible for an application to properly terminate a thread. There's nothing special about that result code that makes it something to handle differently from other result codes, whether the result code is included in a response from the server or triggered by something on the client side. The ResultCode.isConnectionUsable method does allow you to determine whether a non-success result might indicate that the connection is no longer usable.
And regardless of whether the thread were to throw InterruptedException or LDAPException in the event that processing is interrupted, there is no way to resume waiting on the result for the associated operation (unless you're using the asynchronous API), so that operation should be considered a failure.
Thanks again for the response for all the explanations. For me now it is clear how to intearct with SDK and InterruptedException.
Maybe the wording by mail is a bit tricky. When I said imposible, I meant that basically you need to find the problem by youself looking to the source code after you got the error 82... Then of course you can troubleshoot and find the solution and go to the source code. But fora library user maybe could be easier to add your explanations to the Javadoc. To be fair I need to recognize that all annotations you include in the library about thread safety are simply great, so the Interruption handling was the only point I had some doubt about how to manage it properly. So again, thanks gor the great job
I still think it is not according to usual Java practices as decribed in Java Concurrency in Practice (Chapter 7.1.2 Interruption policies page 142) " Whether a task interprets interruption as cancellation or takes some other action on interruption, it should take care to preserve the executing thread’s interruption status. If it is not simply going to propagate InterruptedException to its caller, it should restore the interruption status after catching InterruptedException: Thread.currentThread().interrupt(); " and other links as http://www.javapractices.com/topic/TopicAction.do?Id=251
Anyway, coding it is also about personal styles and of course I fully respect your design choices.
eceejcr is correct: ldapsdk is not handling InterruptedExceptions per well-established Java conventions. Here: https://www.ibm.com/developerworks/library/j-jtp05236/ (IBM, 2006) If you cannot propagate InterruptedException to the caller, and cannot shutdown the thread that was interrupted, then you MUST reassert the interrupted flag. Otherwise, if your APIs are called on an executor that is shutdown, you can cause a graceful shutdown sequence to hang.
I just committed a change that sets the thread's interrupted state in a number of cases where InterruptedException was consumed and not re-thrown.
Thanks Neil. I think it is a good change. If you are ineterested I can test that with my own app and provide feedback to you. Just let me know