Problem with CacheLoader getChildrenNames

[akropp]

I am using a custom CacheLoader to back a TreeCache with a filesystem-based cache -- directories correspond to parent nodes, and files correspond to leaf nodes.

There is a problem with how the CacheLoader is invoked to fetch the children. Let's say my file cache has, and my TreeCache is initially empty:
/a
/a/1
/a/2
/a/3

If I call cache.getChildrenNames("/a"), I will correctly get the child names (1, 2, and 3). However, if I first call cache.get("/a"), and then call cache.getChildrenNames("/a"), I will get an empty set.

I traced it down to this section of CacheLoaderInterceptor:

CacheLoaderInterceptor.java:80
...
 if(fqn != null && (!cache.exists(fqn) || cache.exists(fqn, TreeCache.UNINITIALIZED))) {
 n=loadNode(fqn);
 if(load_attributes && n != null)
 loadAttributesFromCacheLoader(n);
 // n=loadNode(fqn, load_attributes);
 lock(fqn, Node.LOCK_TYPE_WRITE, false); // not recursive
 }

 // special case: node is loaded, but children nodes haven't: check with CacheLoader if children nodes are
 // present and load if yes
 if(meth.equals(TreeCache.getChildrenNamesMethodLocal) && n != null && fqn != null) {
 Map children=n.getChildren();
 if(children == null) {
 Set children_names=null;
 try {
 children_names=loader.getChildrenNames(fqn);
 }
...

The logic in the last bit is that if the node's children haven't been loaded yet, it will get their names from the CacheLoader, and then create uninitialized nodes for each (so future calls to getChildrenNames work). Unfortunately, part of that if() statement checks that n != null, and n only gets set if it did not exist at all, or was uninitialized, in the first part. Thus if the parent node has previously been loaded, n will never get set, and the loader will not get called.

The simple solution is to add an else clause which fetches the already loaded node if it exists. Unfortunately, this just adds a new problem. If I were to first call cache.get("/a/1"), the node now has loaded children. Now if I call cache.getChildrenNames("/a"), it will return me the set (1), which is not consistent with my CacheLoader.

Two possible solutions I can see:

• 
  
• Always stub all of the children of a node whenever the first child is loaded. This will guarantee that no matter what order calls are made to the cache, getChildrenNames will return results consistant with the CacheLoader. Unfortunately, this carries a (potentially large) performance hit, since it requires enumerating all children of a node before fetching one child.
  
• Use a separate flag to indicate whether all children have been loaded/stubbed, or just some (or no) children have been loaded. An additional attribute like TreeCache.UNINITIALIZED could be added, or Node could contain a allChildrenLoaded flag.
  

[belaban]

Can you create a JIRA issue (http://jira.jboss.com/jira/browse/JBCACHE), and attach a unit test ?
Actually, add your unit test to CacheLoaderTestsBase.

Why didn't you use FileCacheLoader (the mapping is the same as the one you describe above) ?
Bela

[belaban]

never mind, I'll do it

[belaban]

I added a test (see below), and it passes. Take a look at let me know what doesn't work.


public void testGetChildren5() {
try {
cache.put("/a/1", null);
cache.put("/a/2", null);
cache.put("/a/3", null);
System.out.println("cache is " + cache.printLockInfo());

Node n=cache.get("/a");
assertNotNull(n);

Set children=cache.getChildrenNames("/a");
assertNotNull(children);
assertEquals(3, children.size());
}
catch(Exception e) {
fail(e.toString());
}
}

[belaban]

Can you create a simple subclass of CacheLoaderTestsBase (see FileCacheLoaderTest as example) and see whether your cache loader impl passes all tests, and especially testGetChildren5() ?

[akropp]

The problem with this test is that you are actively putting the child nodes into the cache. My issue arises when you are relying on the CacheLoader to provide the children. I will try to put together a simple test case tonight when I get home from work.

As far as why I am not using the FileCacheLoader -- I'm not *exactly* mapping the filesystem to the cache. The leaf nodes of my cache are actually dynamically created based on their parents. If my directory looks like:
/a/1
/a/2

There are actually nodes:
/a/1/x
/a/1/y

Which are generated from information pointed to by /a/1.

[belaban]

Okay, so if you can send me the test method so I can integrate it into my testsuite, that would be helpful to fix the problem.

[akropp]

OK, here you go. Three tests, one passes, two fail:

 public void testGetChildren5()
 {
 try {
 cache.put("/a/1", null);
 cache.put("/a/2", null);
 cache.put("/a/3", null);
 System.out.println("cache is " + cache.printLockInfo());
 cache.evict(Fqn.fromString("/a/1"));
 cache.evict(Fqn.fromString("/a/2"));
 cache.evict(Fqn.fromString("/a/3"));
 cache.evict(Fqn.fromString("/a"));
 System.out.println("cache is " + cache.printLockInfo());

 assertNotNull(cache.get("/a"));

 Set children = cache.getChildrenNames("/a");
 assertNotNull("No children were loaded", children);
 System.out.println("children: "+children);
 assertEquals("3 children weren't loaded", 3, children.size());
 } catch (Exception e) {
 fail(e.toString());
 }
 }

 public void testGetChildren6()
 {
 try {
 cache.put("/a/1", null);
 cache.put("/a/2", null);
 cache.put("/a/3", null);
 cache.put("/a", "test", "test");
 System.out.println("cache is " + cache.printLockInfo());
 cache.evict(Fqn.fromString("/a/1"));
 cache.evict(Fqn.fromString("/a/2"));
 cache.evict(Fqn.fromString("/a/3"));
 cache.evict(Fqn.fromString("/a"));
 System.out.println("cache is " + cache.printLockInfo());

 assertEquals("attributes weren't loaded", "test", cache.get("/a", "test"));

 Set children = cache.getChildrenNames("/a");
 assertNotNull("No children were loaded", children);
 System.out.println("children: "+children);
 assertEquals("3 children weren't loaded", 3, children.size());
 } catch (Exception e) {
 fail(e.toString());
 }
 }

 public void testGetChildren7()
 {
 try {
 cache.put("/a/1", null);
 cache.put("/a/2", null);
 cache.put("/a/3", null);
 System.out.println("cache is " + cache.printLockInfo());
 cache.evict(Fqn.fromString("/a/1"));
 cache.evict(Fqn.fromString("/a/2"));
 cache.evict(Fqn.fromString("/a/3"));
 cache.evict(Fqn.fromString("/a"));
 System.out.println("cache is " + cache.printLockInfo());

 assertNull(cache.get("/a","test"));

 cache.get("/a/1");
 Set children = cache.getChildrenNames("/a");
 assertNotNull("No children were loaded", children);
 System.out.println("children: "+children);
 assertEquals("3 children weren't loaded", 3, children.size());
 } catch (Exception e) {
 fail(e.toString());
 }
 }

I tested these out with the FileCacheLoaderTest, but they should happen the same everywhere. Actually, the first one may fail or not depending on the CacheLoader implementation, but the last two always will.

All of them start out the same. I fill up the cache, then evict all of the nodes (so they are still present in the persistent store, but not in memory).

testGetChildren5 is a slightly modified version of what you sent. It calls get("/a") to get the entire /a node, and then calls getChildrenNames(). This seems to work fine.

testGetChildren6 and 7 call get("/a","test"), the form of get which fetches an attribute rather than an entire node. This is where I ran into problems, and this is where it fails. In 6, after I evict all of the nodes, I call get to fetch one attribute from /a. Then I call getChildrenNames, which returns null (should return ([1],[2],[3])). To further illustrate what's happening, in 7 I call get("/a/1") in addition to get("/a","test"). As you can see, it successfully loads the /a/1 node, but now when I call getChildrenNames, it returns me a list with a single element, [1].

To verify that it wasn't the presence or absence of attributes, in 6 I actually added an attribute named "test", while in 7 I didn't. In either case, when I get the attributes of a node, it clears the UNINITIALIZED flag, and then it fails to load the children in the CacheLoaderInterceptor.

[belaban]

Okay, I used the flag approach, although I'm not too happy to have to add a boolean children_loaded to Node. This flag is only used by the CacheLoaderInterceptor, so it breaks separation of concern. Well, for now anyway, it works.
Thanks,

[akropp]

I agree that it is less than ideal. Also, there is an additional concern with any approach that relies on the state of the cache nodes -- if you flag that the children have been loaded, what happens when one of them gets evicted. Does the flag get cleared? For consistancies-sake, it would have to, but this adds another layer of complexity.

An alternative would be for CacheLoaderInterceptor to either always call getChildrenNames for that call (and consequently not have to stub-out the children), or to cache the child names separately from the child nodes. My temporary solution to this was actually along the lines of the first option -- I inserted an extra interceptor before the CacheLoaderInterceptor (I know this is probably bad on all kinds of levels) that traps the getChildrenNames call and always calls it on my CacheLoader.

...or you could remove getChildrenNames from CacheLoader altogether, and state that the call on the cache only returns children that are currently loaded, and avoid this whole mess.

[belaban]

When a node gets evicted, the flag children_loaded goes with it (I made sure of that), so eviction is not a problem.
It works for now, I might revisit this later.

[skotagiri]

Thanks for fixing this bug. Because of this issue, I preload all my cache objects during the initialization and disabled the eviction. That way call to getChildrenNames will always match what is present in my backend store.

In what version can I expect to get this fix? I am currently using JBossCache 1.1 that comes with JBoss 3.2.6.

Thanks.

[belaban]

It is going to be in 1.2.3. I'll release 1.2.3 in the next 2-3 weeks