* Extract cache insertion code into new private registerNewNode() method.

* Delay onInit() invocation until after the node has been registered with the cache.
  Two advantages: first, we won't call onInit() on nodes that are bound to be
  thrown away because a clone already exists in the cache, and second no need
  to cache nodes in the transactor clean node map, which was troublesome in
  exactly the same case.
* Remove some old code that has been commented out for ages.

src/helma/objectmodel/db/Node.java

@@ -17,7 +17,6 @@
 package helma.objectmodel.db;
 
 import helma.framework.IPathElement;
-import helma.framework.core.RequestEvaluator;
 import helma.objectmodel.ConcurrencyException;
 import helma.objectmodel.INode;
 import helma.objectmodel.IProperty;
@@ -165,25 +164,6 @@ public final class Node implements INode, Serializable {
         }
     }
 
-    public void invokeOnInit() {
-        // Invoke onInit() if it is defined by this Node's prototype
-        if (dbmap != null) {
-            // register node with local thread, or else we risk infinite recursion in onInit()
-            if (Thread.currentThread() instanceof Transactor)
-                ((Transactor) Thread.currentThread()).visitCleanNode(this);
-            try {
-                // We need to reach deap into helma.framework.core to invoke onInit(),
-                // but the functionality is neat so we got to be strong here.
-                RequestEvaluator reval = nmgr.nmgr.app.getCurrentRequestEvaluator();
-                if (reval != null) {
-                    reval.invokeDirectFunction(this, "onInit", RequestEvaluator.EMPTY_ARGS);
-                }
-            } catch (Exception x) {
-                nmgr.nmgr.app.logError("Error invoking onInit()", x);
-            }
-        }
-    }
-
     /**
      * Read this object instance from a stream. This does some smart conversion to
      * update from previous serialization formats.

src/helma/objectmodel/db/NodeManager.java

@@ -17,6 +17,7 @@
 package helma.objectmodel.db;
 
 import helma.framework.core.Application;
+import helma.framework.core.RequestEvaluator;
 import helma.objectmodel.*;
 import helma.objectmodel.dom.XmlDatabase;
 
@@ -185,24 +186,14 @@ public final class NodeManager {
                 if (rel != null) {
                     return getNode(parent, key.getID(), rel);
                 } else {
-                    node = null;
+                    return null;
                 }
             } else if (key instanceof DbKey) {
                 node = getNodeByKey(tx.txn, (DbKey) key);
             }
 
             if (node != null) {
-                // synchronize with cache
+                node = registerNewNode(node, null);
-                synchronized (cache) {
-                    Node oldnode = (Node) cache.put(node.getKey(), node);
-
-                    if ((oldnode != null) && !oldnode.isNullNode() &&
-                            (oldnode.getState() != Node.INVALID)) {
-                        cache.put(node.getKey(), oldnode);
-                        node = oldnode;
-                    }
-                }
-                // end of cache-synchronized section
             }
         }
 
@@ -290,32 +281,17 @@ public final class NodeManager {
             node = getNodeByRelation(tx.txn, home, kstr, rel);
 
             if (node != null) {
-                Key primKey = node.getKey();
+                Node newNode = node;
-                boolean keyIsPrimary = primKey.equals(key);
+                if (key.equals(node.getKey())) {
-
+                    node = registerNewNode(node, null);
-                synchronized (cache) {
+                } else {
-                    // check if node is already in cache with primary key
+                    node = registerNewNode(node, key);
-                    Node oldnode = (Node) cache.put(primKey, node);
+                }
-
+                // reset create time of old node, otherwise Relation.checkConstraints
-                    // no need to check for oldnode != node because we fetched a new node from db
+                // will reject it under certain circumstances.
-                    if ((oldnode != null) && !oldnode.isNullNode() &&
+                if (node != newNode) {
-                            (oldnode.getState() != Node.INVALID)) {
+                    node.created = node.lastmodified;
-                        // reset create time of old node, otherwise Relation.checkConstraints
-                        // will reject it under certain circumstances.
-                        oldnode.created = oldnode.lastmodified;
-                        cache.put(primKey, oldnode);
-
-                        if (!keyIsPrimary) {
-                            cache.put(key, oldnode);
-                        }
-
-                        node = oldnode;
-                    } else if (!keyIsPrimary) {
-                        // cache node with secondary key
-                        cache.put(key, node);
-                    }
                 }
-                 // synchronized
             } else {
                 // node fetched from db is null, cache result using nullNode
                 synchronized (cache) {
@@ -332,13 +308,13 @@ public final class NodeManager {
             // update primary key in cache to keep it from being flushed, see above
             if (!rel.usesPrimaryKey()) {
                 synchronized (cache) {
-                    Node oldnode = (Node) cache.put(node.getKey(), node);
+                    Node old = (Node) cache.put(node.getKey(), node);
 
-                    if ((oldnode != node) && (oldnode != null) &&
+                    if (old != node && old != null && !old.isNullNode() && 
-                            (oldnode.getState() != Node.INVALID)) {
+                            old.getState() != Node.INVALID) {
-                        cache.put(node.getKey(), oldnode);
+                        cache.put(node.getKey(), old);
-                        cache.put(key, oldnode);
+                        cache.put(key, old);
-                        node = oldnode;
+                        node = old;
                     }
                 }
             }
@@ -348,7 +324,46 @@ public final class NodeManager {
             tx.visitCleanNode(key, node);
         }
 
-        // tx.timer.endEvent ("getNode "+kstr);
+        return node;
+    }
+
+    /**
+     * Register a newly created node in the node cache unless it it is already contained.
+     * If so, the previously registered node is kept and returned. Otherwise, the onInit()
+     * function is called on the new node and it is returned.
+     * @param node the node to register
+     * @return the newly registered node, or the one that was already registered with the node's key
+     */
+    private Node registerNewNode(Node node, Key secondaryKey) {
+        Key key = node.getKey();
+
+        synchronized(cache) {
+            Node old = (Node) cache.put(key, node);
+
+            if (old != null && !old.isNullNode() && old.getState() != INode.INVALID) {
+                cache.put(key, old);
+                if (secondaryKey != null) {
+                    cache.put(secondaryKey, old);
+                }
+                return old;
+            } else if (secondaryKey != null) {
+                cache.put(secondaryKey, node);
+            }
+        }
+        // New node is going ot be used, invoke onInit() on it
+        // Invoke onInit() if it is defined by this Node's prototype
+        if (node.dbmap != null) {
+            try {
+                // We need to reach deap into helma.framework.core to invoke onInit(),
+                // but the functionality is really worth it.
+                RequestEvaluator reval = app.getCurrentRequestEvaluator();
+                if (reval != null) {
+                    reval.invokeDirectFunction(node, "onInit", RequestEvaluator.EMPTY_ARGS);
+                }
+            } catch (Exception x) {
+                app.logError("Error invoking onInit()", x);
+            }
+        }
         return node;
     }
 
@@ -359,7 +374,6 @@ public final class NodeManager {
         cache.put(node.getKey(), node);
     }
 
-
     /**
      * Register a node in the node cache using the key argument.
      */
@@ -410,8 +424,6 @@ public final class NodeManager {
      */
     public void insertNode(IDatabase db, ITransaction txn, Node node)
                     throws IOException, SQLException, ClassNotFoundException {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("insertNode "+node);
         DbMapping dbm = node.getDbMapping();
 
         if ((dbm == null) || !dbm.isRelational()) {
@@ -530,8 +542,6 @@ public final class NodeManager {
      */
     public boolean updateNode(IDatabase db, ITransaction txn, Node node)
                     throws IOException, SQLException, ClassNotFoundException {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("updateNode "+node);
         DbMapping dbm = node.getDbMapping();
         boolean markMappingAsUpdated = false;
 
@@ -746,8 +756,6 @@ public final class NodeManager {
      */
     synchronized String generateMaxID(DbMapping map)
                                       throws Exception {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("generateID "+map);
         String retval = null;
         Statement stmt = null;
         long logTimeStart = logSql ? System.currentTimeMillis() : 0;
@@ -794,8 +802,6 @@ public final class NodeManager {
     }
 
     String generateSequenceID(DbMapping map) throws Exception {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("generateID "+map);
         Statement stmt = null;
         String retval = null;
         long logTimeStart = logSql ? System.currentTimeMillis() : 0;
@@ -846,8 +852,6 @@ public final class NodeManager {
      *  loaded later on demand.
      */
     public SubnodeList getNodeIDs(Node home, Relation rel) throws Exception {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("getNodeIDs "+home);
 
         if ((rel == null) || (rel.otherType == null) || !rel.otherType.isRelational()) {
             // this should never be called for embedded nodes
@@ -920,7 +924,6 @@ public final class NodeManager {
                     Key key = (rel.groupby == null)
                               ? (Key) new DbKey(rel.otherType, kstr)
                               : (Key) new SyntheticKey(k, kstr);
-
                     retval.addSorted(new NodeHandle(key));
 
                     // if these are groupby nodes, evict nullNode keys
@@ -961,8 +964,6 @@ public final class NodeManager {
             return getNodeIDs(home, rel);
         }
 
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("getNodes "+home);
         if ((rel == null) || (rel.otherType == null) || !rel.otherType.isRelational()) {
             // this should never be called for embedded nodes
             throw new RuntimeException("NodeMgr.getNodes called for non-relational node " +
@@ -1013,14 +1014,7 @@ public final class NodeManager {
 
                     retval.addSorted(new NodeHandle(primKey));
 
-                    // do we need to synchronize on primKey here?
+                    registerNewNode(node, null);
-                    synchronized (cache) {
-                        Node oldnode = (Node) cache.put(primKey, node);
-
-                        if ((oldnode != null) && (oldnode.getState() != INode.INVALID)) {
-                            cache.put(primKey, oldnode);
-                        }
-                    }
 
                     fetchJoinedNodes(rs, joins, columns.length);
                 }
@@ -1167,12 +1161,7 @@ public final class NodeManager {
                             continue;
                         }
                         key = node.getKey();
-                        synchronized (cache) {
+                        registerNewNode(node, null);
-                            Node oldnode = (Node) cache.put(key, node);
-                            if ((oldnode != null) && (oldnode.getState() != INode.INVALID)) {
-                                cache.put(key, oldnode);
-                            }
-                        }
                     } else {
                         key = new DbKey(rel.otherType, kstr);
                     }
@@ -1271,7 +1260,8 @@ public final class NodeManager {
                         if (node == null) {
                             continue;
                         }
-                        Key primKey = node.getKey();
+                        Key key = node.getKey();
+                        Key secondaryKey = null;
 
                         // for grouped nodes, collect subnode lists for the intermediary
                         // group nodes.
@@ -1287,37 +1277,26 @@ public final class NodeManager {
                                 groupbySubnodes.put(groupName, sn);
                             }
 
-                            sn.addSorted(new NodeHandle(primKey));
+                            sn.addSorted(new NodeHandle(key));
                         }
 
-                        // if relation doesn't use primary key as accessName, get accessName value
+                        // if relation doesn't use primary key as accessName, get secondary key
-                        String accessName = null;
-
                         if (accessProp != null) {
-                            accessName = node.getString(accessProp);
+                            String accessName = node.getString(accessProp);
+                            if (accessName != null) {
+                                if (groupName == null) {
+                                    secondaryKey = new SyntheticKey(home.getKey(), accessName);
+                                } else {
+                                    Key groupKey = new SyntheticKey(home.getKey(), groupName);
+                                    secondaryKey = new SyntheticKey(groupKey, accessName);
+                                }
+                            }
+
                         }
 
                         // register new nodes with the cache. If an up-to-date copy
                         // existed in the cache, use that.
-                        synchronized (cache) {
+                        registerNewNode(node, secondaryKey);
-                            Node oldnode = (Node) cache.put(primKey, node);
-
-                            if ((oldnode != null) &&
-                                    (oldnode.getState() != INode.INVALID)) {
-                                // found an ok version in the cache, use it.
-                                cache.put(primKey, oldnode);
-                            } else if (accessName != null) {
-                                // put the node into cache with its secondary key
-                                if (groupName != null) {
-                                    cache.put(new SyntheticKey(new SyntheticKey(home.getKey(),
-                                                                                groupName),
-                                                               accessName), node);
-                                } else {
-                                    cache.put(new SyntheticKey(home.getKey(), accessName),
-                                              node);
-                                }
-                            }
-                        }
 
                         fetchJoinedNodes(rs, joins, columns.length);
                     }
@@ -1363,8 +1342,6 @@ public final class NodeManager {
      * which is defined by Relation rel.
      */
     public int countNodes(Node home, Relation rel) throws Exception {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("countNodes "+home);
         if ((rel == null) || (rel.otherType == null) || !rel.otherType.isRelational()) {
             // this should never be called for embedded nodes
             throw new RuntimeException("NodeMgr.countNodes called for non-relational node " +
@@ -1436,8 +1413,6 @@ public final class NodeManager {
      */
     public Vector getPropertyNames(Node home, Relation rel)
                             throws Exception {
-        // Transactor tx = (Transactor) Thread.currentThread ();
-        // tx.timer.beginEvent ("getNodeIDs "+home);
         if ((rel == null) || (rel.otherType == null) || !rel.otherType.isRelational()) {
             // this should never be called for embedded nodes
             throw new RuntimeException("NodeMgr.getPropertyNames called for non-relational node " +
@@ -1554,7 +1529,6 @@ public final class NodeManager {
                 if (!rs.next()) {
                     return null;
                 }
-
                 node = createNode(dbm, rs, columns, 0);
 
                 fetchJoinedNodes(rs, joins, columns.length);
@@ -1665,16 +1639,6 @@ public final class NodeManager {
                     app.logError("Warning: More than one value returned for query " + query);
                 }
 
-                // Check if node is already cached with primary Key.
-                if (!rel.usesPrimaryKey()) {
-                    Key pk = node.getKey();
-                    Node existing = (Node) cache.get(pk);
-
-                    if ((existing != null) && (existing.getState() != Node.INVALID)) {
-                        node = existing;
-                    }
-                }
-
             } finally {
                 if (logSql) {
                     long logTimeStop = System.currentTimeMillis();
@@ -1889,7 +1853,6 @@ public final class NodeManager {
         }
 
         node.init(dbmap, id, name, protoName, propMap, safe);
-        node.invokeOnInit();
         return node;
     }
 
@@ -1904,18 +1867,7 @@ public final class NodeManager {
             DbMapping jdbm = joins[i].otherType;
             Node node = createNode(jdbm, rs, jdbm.getColumns(), resultSetOffset);
             if (node != null) {
-                Key primKey = node.getKey();
+                registerNewNode(node, null);
-                // register new nodes with the cache. If an up-to-date copy
-                // existed in the cache, use that.
-                synchronized (cache) {
-                    Node oldnode = (Node) cache.put(primKey, node);
-
-                    if ((oldnode != null) &&
-                            (oldnode.getState() != INode.INVALID)) {
-                        // found an ok version in the cache, use it.
-                        cache.put(primKey, oldnode);
-                    }
-                }
             }
             resultSetOffset += jdbm.getColumns().length;
         }

src/helma/objectmodel/dom/XmlDatabaseReader.java

@@ -198,9 +198,7 @@ public final class XmlDatabaseReader extends DefaultHandler implements XmlConsta
      */
     public void endElement(String namespaceURI, String localName, String qName)
                     throws SAXException {
-        if ("hopobject".equals(qName) && currentNode != null) {
+        if (elementType != null) {
-            currentNode.invokeOnInit();
-        } else if (elementType != null) {
             Property prop = new Property(elementName, currentNode);
             String charValue = charBuffer.toString();