tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: JNDIRealm performance
Date Fri, 17 Feb 2012 16:33:00 GMT
Konstantin,

On 2/14/12 6:55 PM, Konstantin Kolinko wrote:
> I wonder whether anyone experienced problems with performance of JNDIRealm.
> 
> In essence it opens a single connection  (this.context) which is not
> thread-safe. Thus it has to synchronize itself whenever it performs an
> LDAP query.

So, JNDIRealm is the new JDBCRealm? :)

> I see a race condition in the open() method. Both open() and close()
> update this.context, but they are not synchronized.
> There are several methods that call them, most notably
> authenticate(String,String).
> The JNDIRealm#startInternal() pre-opens context at startup, so this
> would be hard to observe,  but once the connection fails I expect a
> bunch of concurrent calls to open() from authenticate(String,String),
> with all of them but one being wasted.

That sounds about right.

> I think it can be fixed by adding synchronized modifier to both open()
> and close(), but I am afraid that it will make the things worse.

Without contention, there is no problem. Under load with a good
connection, I don't think it will represent a significant problem
because open() would return fairly quickly. The only problem is under
load with a broken connection: current implementation tries to connect
many times and the last one wins. Depending on a thread's cached copy of
the 'context' member, each call to authenticate(DirContext,...) might
get a shared connection or not. At any rate,
authenticate(DirContext,...) is synchronized, and that's where the real
work gets done.

> If there will be a simple synchronization it means that the threads
> will wait on the monitor instead of wasting their time in connection
> attempt: If the LDAP server disappears it may lock all request
> processing threads: They will wait on the same monitor and then try
> two connection attempts each.

The current open() method checks for a null context, so if one thread
succeeds, the queued calls to open() will of course clear quickly. If
the LDAP server dies, then yes each thread will repeatedly try to connect.

How is this any different than a DataSourceRealm with a connection pool
size of 1? Each client thread tries to contact the database, fails, and
then the next one in line does the same thing (or the first one retries
while all the others wait -- it's the same effect).

IMO that class *could* use an upgrade (pooling would be nice) and some
clean-up (e.g. credentials are always decoded into bytes using
ISO_8859_1 and not using getDigestEncoding -- maybe that's an LDAP spec
but then it should be documented as such).

-chris


Mime
View raw message