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
signature.asc
Description: OpenPGP digital signature
