Re: [tomcat] 01/04: Add connection pool to JNDI realm
On Tue, Oct 13, 2020 at 8:26 PM Michael Osipov wrote: > Am 2020-10-13 um 13:49 schrieb Rémy Maucherat: > > On Tue, Oct 13, 2020 at 11:33 AM Michael Osipov > 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 > >>> 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 ? > > I consider it cheaper to test for a timeout and close actively rather > than recover from a resetted connection. Here is a strack trace for > concurrency of 4 (with a pool of max 8). Compare the timestamp of the > first line and the subsequent ones: > > > 2020-10-12T22:55:47 FEIN [https-openssl-apr-8444-exec-8] > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.release Releasing > directory server connection to pool > > 2020-10-12T23:13:06 FEIN [https-openssl-apr-8444-exec-5] > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.acquire Acquiring > directory server connection from pool > > 2020-10-12T23:13:06 FEIN [https-openssl-apr-8444-exec-5] > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.validate Validating > directory server connection from pool > > 2020-10-12T23:13:06 SCHWERWIEGEND [https-openssl-apr-8444-exec-5] > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.validate Exception > validating directory server connection > > javax.naming.CommunicationException: Connection reset [Root > exception is java.net.SocketException: Connection reset]; remaining name '' > > at com.sun.jndi.ldap.LdapCtx.doSearch(LdapCtx.java:2030) > > at com.sun.jndi.ldap.LdapCtx.searchAux(LdapCtx.java:1872) > > at com.sun.jndi.ldap.LdapCtx.c_search(LdapCtx.java:1797) > > at > com.sun.jndi.toolkit.ctx.ComponentDirContext.p_search(ComponentDirContext.java:392) > > at > com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:358) > > at > com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:341) > > at > javax.naming.directory.InitialDirContext.search(InitialDirContext.java:296) > > at > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.validate(ActiveDirectoryRealm.java:316) > > at > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.acquire(ActiveDirectoryRealm.java:285) > > at > net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.getPrincipal(ActiveDirectoryRealm.java:244) > > at > org.apache.catalina.realm.RealmBase.authenticate(RealmBase.java:501) > > at > net.sf.michaelo.tomcat.authenticator.SpnegoAuthenticator.doAuthenticate(SpnegoAuthenticator.java:165) > > at > org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:633) > >
Re: [tomcat] 01/04: Add connection pool to JNDI realm
Am 2020-10-13 um 13:49 schrieb Rémy Maucherat: On Tue, Oct 13, 2020 at 11:33 AM Michael Osipov 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 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 ? I consider it cheaper to test for a timeout and close actively rather than recover from a resetted connection. Here is a strack trace for concurrency of 4 (with a pool of max 8). Compare the timestamp of the first line and the subsequent ones: 2020-10-12T22:55:47 FEIN [https-openssl-apr-8444-exec-8] net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.release Releasing directory server connection to pool 2020-10-12T23:13:06 FEIN [https-openssl-apr-8444-exec-5] net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.acquire Acquiring directory server connection from pool 2020-10-12T23:13:06 FEIN [https-openssl-apr-8444-exec-5] net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.validate Validating directory server connection from pool 2020-10-12T23:13:06 SCHWERWIEGEND [https-openssl-apr-8444-exec-5] net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.validate Exception validating directory server connection javax.naming.CommunicationException: Connection reset [Root exception is java.net.SocketException: Connection reset]; remaining name '' at com.sun.jndi.ldap.LdapCtx.doSearch(LdapCtx.java:2030) at com.sun.jndi.ldap.LdapCtx.searchAux(LdapCtx.java:1872) at com.sun.jndi.ldap.LdapCtx.c_search(LdapCtx.java:1797) at com.sun.jndi.toolkit.ctx.ComponentDirContext.p_search(ComponentDirContext.java:392) at com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:358) at com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:341) at javax.naming.directory.InitialDirContext.search(InitialDirContext.java:296) at net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.validate(ActiveDirectoryRealm.java:316) at net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.acquire(ActiveDirectoryRealm.java:285) at net.sf.michaelo.tomcat.realm.ActiveDirectoryRealm.getPrincipal(ActiveDirectoryRealm.java:244) at org.apache.catalina.realm.RealmBase.authenticate(RealmBase.java:501) at net.sf.michaelo.tomcat.authenticator.SpnegoAuthenticator.doAuthenticate(SpnegoAuthenticator.java:165) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:633) at org.apache.catalina.valves.rewrite.RewriteValve.invoke(RewriteValve.java:571) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81) at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:690) at
Re: [tomcat] 01/04: Add connection pool to JNDI realm
On Tue, Oct 13, 2020 at 1:49 PM Rémy Maucherat wrote: 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. > Done. If it doesn't work, feel free to add more refinements. Otherwise, I'll backport. Rémy
Re: [tomcat] 01/04: Add connection pool to JNDI realm
On Tue, Oct 13, 2020 at 11:33 AM Michael Osipov 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
> > 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 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();
> >
> >
Re: [tomcat] 01/04: Add connection pool to JNDI realm
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
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)
* 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
* 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 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
* 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 =
[tomcat] 01/04: Add connection pool to JNDI realm
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
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(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
b/java/org/apache/catalina/realm/JNDIRealm.java
index a9565dc..a22e3c6 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -32,6 +32,8 @@ import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
import javax.naming.AuthenticationException;
import javax.naming.CommunicationException;
@@ -61,6 +63,7 @@ import javax.net.ssl.SSLSession;
import javax.net.ssl.SSLSocketFactory;
import org.apache.catalina.LifecycleException;
+import org.apache.tomcat.util.collections.SynchronizedStack;
import org.ietf.jgss.GSSCredential;
import org.ietf.jgss.GSSName;
@@ -166,10 +169,6 @@ import org.ietf.jgss.GSSName;
* directory server itself.
*
*
- * TODO - Support connection pooling (including message
- * format objects) so that authenticate() does not have to be
- * synchronized.
- *
* WARNING - There is a reported bug against the Netscape
* provider code (com.netscape.jndi.ldap.LdapContextFactory) with respect to
* successfully authenticated a non-existing user. The
@@ -209,12 +208,6 @@ public class JNDIRealm extends RealmBase {
/**
- * The directory context linking us to our directory server.
- */
-protected DirContext context = null;
-
-
-/**
* The JNDI context factory used to acquire our InitialContext. By
* default, assumes use of an LDAP server using the standard JNDI LDAP
* provider.
@@ -291,13 +284,6 @@ public class JNDIRealm extends RealmBase {
/**
- * The MessageFormat object associated with the current
- * userSearch.
- */
-protected MessageFormat userSearchFormat = null;
-
-
-/**
* Should we search the entire subtree for matching users?
*/
protected boolean userSubtree = false;
@@ -337,32 +323,12 @@ public class JNDIRealm extends RealmBase {
/**
- * An array of MessageFormat objects associated with the current
- * userPatternArray.
- */
-protected MessageFormat[] userPatternFormatArray = null;
-
-/**
* The base element for role searches.
*/
protected String roleBase = "";
/**
- * The MessageFormat object associated with the current
- * roleBase.
- */
-protected MessageFormat roleBaseFormat = null;
-
-
-/**
- * The MessageFormat object associated with the current
- * roleSearch.
- */
-protected MessageFormat roleFormat = null;
-
-
-/**
* The name of an attribute in the user's entry containing
* roles for that user
*/
@@ -508,6 +474,30 @@ public class JNDIRealm extends RealmBase {
private boolean forceDnHexEscape = false;
+/**
+ * Non pooled connection to our directory server.
+ */
+protected JNDIConnection singleConnection = new JNDIConnection();
+
+
+/**
+ * The lock to ensure single connection thread safety.
+ */
+protected final Lock singleConnectionLock = new ReentrantLock();
+
+
+/**
+ * Connection pool.
+ */
+protected SynchronizedStack connectionPool = null;
+
+
+/**
+ * The pool size limit. If 1, pooling is not used.
+ */
+protected int connectionPoolSize = 1;
+
+
// - Properties
public boolean getForceDnHexEscape() {
@@ -736,13 +726,8 @@ public class JNDIRealm extends RealmBase {
* @param userSearch The new user search pattern
*/
public void setUserSearch(String userSearch) {
-
