On Tue, Oct 13, 2020 at 11:33 AM Michael Osipov <micha...@apache.org> wrote:
> Am 2020-10-07 um 22:34 schrieb r...@apache.org: > > This is an automated email from the ASF dual-hosted git repository. > > > > remm pushed a commit to branch 8.5.x > > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > > > commit 50de36b7874da98591345e40b374a1e2dd52c188 > > Author: remm <r...@apache.org> > > AuthorDate: Thu Jan 30 17:22:51 2020 +0100 > > > > Add connection pool to JNDI realm > > > > This implements a TODO from the class javadoc header. > > As described in the javadoc, the idea is to use a pool to avoid > blocking > > on a single connection, which could possibly become a bottleneck in > some > > cases. The message formats need to be kept along with the connection > > since they are not thread safe. > > Preserve the default behavior: sync without pooling (using a Lock > object > > which is more flexible). > > I may backport this since this is limited to the JNDI realm, but > only > > once it is confirmed to be regression free. Tested with ApacheDS > but my > > LDAP skills are very limited. > > --- > > java/org/apache/catalina/realm/JNDIRealm.java | 442 > ++++++++++++--------- > > .../apache/catalina/realm/LocalStrings.properties | 1 + > > test/org/apache/catalina/realm/TestJNDIRealm.java | 7 +- > > webapps/docs/changelog.xml | 3 + > > webapps/docs/config/realm.xml | 7 + > > 5 files changed, 276 insertions(+), 184 deletions(-) > > Salut Rémy, > > this is a very very nice improvement to the matter and I gave your idea > a spin my public Active Directory realm. Based on my tests with AD I see > room for improvement: (Note that I don't use the JNDIRealm because > because is it too generic and usable for me) > Ok, I only tested this with a local ldap, so not much. I verified it was pooling connections. > > * You might want to consider to introduce a maxIdleTime to avoid stale > connections, e.g., AD has default value of 15 minutes. Yep, I had had > several RSTs resulting in 401s > Oops. So I don't think adding something like this helps since there could be plenty of issues besides a timeout. Basically it's supposed to retry instead when something fails. Can you give me a stack trace to show what's wrong ? https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/realm/JNDIRealm.java#L1306 Looking at the code, it does retry, but it would simply get a new connection from the pool again. So if it gets a second bad connection, then it would most likely fail. Is it what happened for you ? A solution could be to create a new connection for the pooled case, to make up for that. > * Validating connections optionally might be a good option to detect > stale connections or broken ones ny networks issues. This works for me > very fast: > > protected boolean validate(DirContextConnection connection) { > > if (logger.isDebugEnabled()) > > > logger.debug(sm.getString("activeDirectoryRealm.validate")); > > > > SearchControls controls = new SearchControls(); > > controls.setSearchScope(SearchControls.OBJECT_SCOPE); > > controls.setCountLimit(1); > > controls.setReturningAttributes(new String[] { > "objectClass" }); > > controls.setTimeLimit(500); > > > > try { > > NamingEnumeration<SearchResult> results = > connection.context.search("", "objectclass=*", > > controls); > > > > if (results.hasMore()) { > > close(results); > > return true; > > } > > } catch (NamingException e) { > > > logger.error(sm.getString("activeDirectoryRealm.validate.namingException"), > e); > > > > return false; > > } > > > > return false; > > } > I do this in the acquire() method > Ok, but it has a cost and I fundamentally don't see why it's different from performing the actual operation. > > * The acquire(), authenticate(), fail, close(), authenticate(), > release() is brittle and proved to fail here. I have a curl script which > does 4 requests in parallel and 1200 requests in total. Pool size is 8. > 4 connections in the pool. Rest for 16 minutes, connections are broken > now. Start requests. authenticate() will fail twice because top two > acquired connections from pool are broken. Requests end with 401. I'd > prefer to to modify acquire() is such a fashion: > > protected DirContextConnection acquire() { > > if (logger.isDebugEnabled()) > > > logger.debug(sm.getString("activeDirectoryRealm.acquire")); > > > > DirContextConnection connection = null; > > > > while (connection == null) { > > connection = connectionPool.pop(); > > > > if (connection != null) { > > long idleTime = System.currentTimeMillis() > - connection.lastBorrowTime; > > if (idleTime > maxIdleTime) { > > if (logger.isDebugEnabled()) > > > logger.debug(sm.getString("activeDirectoryRealm.exceedMaxIdleTime")); > > close(connection); > > connection = null; > > } else { > > boolean valid = > validate(connection); > > if (valid) { > > if > (logger.isDebugEnabled()) > > > logger.debug(sm.getString("activeDirectoryRealm.reuse")); > > } else { > > close(connection); > > connection = null; > > } > > } > > } else { > > connection = new DirContextConnection(); > > open(connection); > > } > > } > > > WDYT? > That it's the issue. However, since the realm works well in the single connection case, I'd just put that loop in authenticate(String username, String credentials), keep the same simple design and be done with it. Rémy
