LDAP connection pools are not freeing connections when shut down
Description
Environment
Activity
Fix for v5 at: http://git.shibboleth.net/view/?p=java-shib-attribute.git;a=commitdiff;h=75126c7a92b503a2eabdcd1479047d4bb1832cbc
v5 includes an API change to the Validator interface to accept the DataConnector being validated. (Still not completely stateless)
Fix for v4 at: http://git.shibboleth.net/view/?p=java-identity-provider.git;a=commitdiff;h=cdd7299a5a314bcf2166b6d9dd6a605a10a63a35
Data connectors pass their resource reference to their validator when the resource setter is called. Bit of a code smell, but I could not find a Spring based solution.
Notable changes for both v4 and v5:
LDAP and RDBMS validators no longer extend AbstractInitializableComponent
LDAP and RDBMS data connectors mutate their validators so that throwOnValidatorError = failsFastInitialize during initialize
Note that the LDAP validator defaults throwOnValidatorError to false, while the RDBMS validator default throwOnValidatorError to true. I left that alone, I’m assuming there is some history there I’m unaware of.
I can see both sides of the argument, but I wonder if it’s a little odd to have to templatize the abstract connector base class with the actual type of the connector. Seems kind of weird.
Generics being such a mess, I wonder if it suffices to just make the Vallidator interface take DataConnector and let them cast down to see whether they should work. It seems ok to me to have an LDAP connection validator throw if it’s handed an RDBMS connector, etc. It’s not going to happen anyway.
Pushed up a potential fix at:
I did not address the inconsistencies in the LDAP and RDBMS validator usage yet. This patch was large enough that I felt the work merited its own focused commit.
If everyone is ok with the Validator interface change, I’ll merge in a few days.
For clarification, I’m not actively working on that code, I just happened to pull the component interface when I looked over that code, on the theory that if we’re not calling initialize, we shouldn’t have the interface there.
@Daniel Fisher I don’t have that stuff swapped in right now but it makes sense - but it all devolves to when the validators are called (if for instance they are called outside of initialize then throwValidateError=true becomes unrelated to failFastinitialize=true
Its seems that throwValidateErrors implies failFastInitialize but not vice versa..
Steve Mak reported that reloading the resolver was accumulating LDAP connections and I verified that on my IdP. I added code to close the connection pool on bean destroy but despite it logging that it was called, the connections are still there.