ldapsdk should stop suppressing `Error`

[jaymode]

The ldapsdk currently has exception handling blocks that catch Throwable and then anything that is not an LDAPException, including Error instances, gets wrapped in an LDAPException. One such place in the code is:

ldapsdk/src/com/unboundid/ldap/sdk/LDAPConnectionPool.java

Lines 1623 to 1675 in 0eac394

	catch (final Throwable t)
	{
	Debug.debugException(t);
	if (t instanceof LDAPException)
	{
	final LDAPException le = (LDAPException) t;
	boolean shouldThrow;
	try
	{
	healthCheck.ensureConnectionValidAfterException(conn, le);
	// The above call will throw an exception if the connection doesn't
	// seem to be valid, so if we've gotten here then we should assume
	// that it is valid and we will pass the exception onto the client
	// without retrying the operation.
	releaseAndReAuthenticateConnection(conn);
	shouldThrow = true;
	}
	catch (final Exception e)
	{
	Debug.debugException(e);
	// This implies that the connection is not valid. If the pool is
	// configured to re-try bind operations on a newly-established
	// connection, then that will be done later in this method.
	// Otherwise, release the connection as defunct and pass the bind
	// exception onto the client.
	if (! getOperationTypesToRetryDueToInvalidConnections().contains(
	OperationType.BIND))
	{
	releaseDefunctConnection(conn);
	shouldThrow = true;
	}
	else
	{
	shouldThrow = false;
	}
	}
	if (shouldThrow)
	{
	throw le;
	}
	}
	else
	{
	releaseDefunctConnection(conn);
	throw new LDAPException(ResultCode.LOCAL_ERROR,
	ERR_POOL_OP_EXCEPTION.get(StaticUtils.getExceptionMessage(t)), t);
	}
	}

There are other places in the code where Throwable is caught:

$ git rev-parse HEAD
0eac394a077323485b850a90e55f926bc035023f
$ ack --type java "catch\s*\(\s*(final)?\s*Throwable" | wc -l
      43

Details

$ ack --type java "catch\s*\(\s*(final)?\s*Throwable" tests/unit/src/com/unboundid/test/LDAPSDKTestListener.java 127: catch (final Throwable t)

tests/unit/src/com/unboundid/util/WakeableSleeperTestCaseHelperThread.java
182: catch (Throwable t)

tests/unit/src/com/unboundid/ldap/sdk/migrate/ldapjdk/LDAPConnectionTestCase.java
102: } catch (Throwable t) {}
162: } catch (Throwable t) {}
377: } catch (Throwable t) {}

tests/unit/src/com/unboundid/ldap/sdk/DSEETestCase.java
102: catch (Throwable t)

src/com/unboundid/util/parallel/ParallelProcessor.java
348: catch (final Throwable e)
377: catch (final Throwable e)

src/com/unboundid/util/parallel/AsynchronousParallelProcessor.java
291: catch (final Throwable e)

src/com/unboundid/ldap/listener/LDAPListenerClientConnection.java
624: catch (final Throwable t)

src/com/unboundid/ldap/sdk/ConnectThread.java
151: catch (final Throwable t)

src/com/unboundid/ldap/sdk/AbstractConnectionPool.java
458: catch (final Throwable t)
472: catch (final Throwable t2)
540: catch (final Throwable t)
554: catch (final Throwable t2)
619: catch (final Throwable t)
633: catch (final Throwable t2)
765: catch (final Throwable t)
779: catch (final Throwable t2)
869: catch (final Throwable t)
883: catch (final Throwable t2)
948: catch (final Throwable t)
962: catch (final Throwable t2)
1043: catch (final Throwable t)
1057: catch (final Throwable t2)
1181: catch (final Throwable t)
1196: catch (final Throwable t2)
1328: catch (final Throwable t)
1342: catch (final Throwable t2)
1460: catch (final Throwable t)
1474: catch (final Throwable t2)
2060: catch (final Throwable t)
2083: catch (final Throwable t2)
2429: catch (final Throwable t)
2452: catch (final Throwable t2)

src/com/unboundid/ldap/sdk/LDAPConnectionPool.java
1623: catch (final Throwable t)
1688: catch (final Throwable t)

src/com/unboundid/ldap/sdk/persist/DefaultObjectEncoder.java
1251: catch (final Throwable t)

src/com/unboundid/ldap/sdk/persist/LDAPObjectHandler.java
1233: catch (final Throwable t)
1302: catch (final Throwable t)
1401: catch (final Throwable t)

src/com/unboundid/ldap/sdk/LDAPThreadLocalConnectionPool.java
721: catch (final Throwable t)
786: catch (final Throwable t)

Some of these cases are in tests but from what I see the majority are not in tests. I did not inspect each case for the behavior of wrapping but I believe each one should be looked at to see if it indeed wraps and hides Errors.

The Javadocs for Error state:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

While it is reasonable to catch a Throwable and do some cleanup, if the Throwable is an Error it should not be suppressed by wrapping it in an LDAPException. Errors should bubble up to the application's handling (if any); in the Elasticsearch case if we hit an error, we want to "die with dignity" (see elastic/elasticsearch#19272).

[dirmgr]

This should be addressed by the changes for commit 9b012d2.

[jaymode]

@dirmgr thanks for addressing so quickly! 👍

[dirmgr]

No problem. Sorry I couldn't get to it yesterday.

[dirmgr]

I have just released the 4.0.8 version of the LDAP SDK that includes this fix.