Working my way up the stack

[genman]

1. ./src/org/jboss/cache/interceptors/CacheLoaderInterceptor.java

in invoke():

Why does TreeCache.putDataEraseMethodLocal and TreeCache.putDataMethodLocal and removeDataLocal have to load the attributes? There are no values returned from these operations.

2. src/org/jboss/cache/interceptors/TxInterceptor.java

 private GlobalTransaction registerTransaction(Transaction tx) throws Exception
 if (!transactions.containsKey(tx) && isValid(tx))
 {
 gtx = cache.getCurrentTransaction(tx);
 if (gtx.isRemote())
 {
 // should be no need to register a handler since this a remotely initiated gtx
 if (log.isTraceEnabled());
 }
 else
 {
 if (log.isTraceEnabled());
 LocalSynchronizationHandler myHandler = new ...
 }
 transactions.put(tx,NULL);

Seems like a race condition between the "contains" and "put". Too bad there's no "putIfAbsent" operation in the ConcurrentReaderHashMap. This should work:

 if (isValid(tx) && transactions.put(tx, NULL) == null)
 {

3. src/org/jboss/cache/interceptors/PessimisticLockInterceptor.java

line 340 and 260

 List list=new LinkedList(entry.getLocks());
 for(int i=list.size() - 1; i >= 0; i--) {
 IdentityLock lock=(IdentityLock)list.get(i);

seems a bit odd, why not an ArrayList or a Reverse ListIterator? (See above)

Also,

Thread.currentThread()

isn't free, if you use it more than once, store it in a local.

4. src/org/jboss/cache/interceptors/CacheMgmtInterceptor.java

 private int m_seq = 0;
 private int m_listeners = 0;

should these be locked? (E.g. SynchronizedInt ?)

[manik]

Modified your original post to make it a bit more readable. :-)

[manik]

Re: #2, +1. Makes sense, certainly cleaner and eliminates the potential race cond.

[manik]

Re: #3,

+1 on caching Thread.currentThread() - the cost of this method call is not that great, but when it is done recursively it does add up - especially when it is so easy to fix.

+1 on the loop as well - an unnecessary complexity. Perhaps even a simple

IdentityLock[] locks = (IdentityLock[]) entry.getLocks().toArray(new IdentityLock[]{});

would do the trick

[genman]

How about items 1 and 4?

[manik]

You'd want to hear back from Jerry Gauthier re. 4, who implemented the cache management interceptor.

[manik]

Re: 1, let me have a look and get back to you, although try disabling load attribs for these 2 cases and running the test suite.

[jerrygauth]

re: 4.

a) I think you're right on m_seq. The sequence number must be serialized so that clients can sort on it if necessary. It does seem that the current implementation might be susceptible to errors in a threaded environment.

The m_seq variable should also be redefined as a long (SynchronizedLong?) since this is what the Notification constructor specifies. It seems that I mistakenly defined it as an int.

b) The m_listeners variable is only used within a synchronized method so it appears to be ok as is.