Correctly implementing the Swing TreeModel

Ideally the Code Review would target the correctness of the approach implementing the Swing TreeModel.

In particular, is the structural separation[1], event message passing, threading[2], object synchronisation, etc. all best practice ?

• [1] My understanding is that the TreeModel should have separation between the underlying Tree Model and the object reports changes to the Swing JTree.

• [2] My understanding is that there should be separate threads for UI and Model changes.

In the interests of disclosure there is a bug that the UI doesn't show new child nodes once the parent node has been expanded in the UI.

Once any issues with the correctness of the approach are sorted, and if the bug still exits, assistance in this area would be appreciated.

Full Runnable Package is on Github

Aside: Please note I'm still pretty new to this Code Review site, Swing and Java.
I have read and tried to follow the question guidelines.

CLASS: Node - A Node in the Tree Model.

```
/** This document is AS-IS. No claims are made for suitability for any purpose. */
package com.example.mutablejtreemodel;

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.WeakHashMap;
import java.util.logging.Logger;
import javax.swing.tree.TreePath;

/**
* A node in a tree structure.
*
* Nodes will fire change events to listeners which can be other nodes, or
* JTreeModel objects.
*
* @author xenomorpheus
* @version $Revision: 1.0 $
*/
public class Node implements ActionListener {

/** class logger */
private static final Logger LOGGER = Logger.getLogger(Node.class.getName());

/** synchronisation lock */
private static final Object OBJ_LOCK = new Object();
// Read more:
// http://javarevisited.blogspot.com/2011/04/synchronization-in-java-synchronized.html#ixzz2wy76gzSj

/** The human identifiable name of this node. */
private String name;

/** Our p

static

What a difference 1 word can make:

/** synchronisation lock */
private static final Object OBJ_LOCK = new Object();

That 1 word is 'static'.

In this case, each and every Node shares the exact same OBJ_LOCK instance.

But, that instance is only used to control details inside the actual Node.

What you are doing is locking all threads accessing ANY Node, even though you are only changing the details in one node.

Remove the static, and change the case to objLock, and you should have better thread concurrency.
Listeners

private void fireNodeChanged(ActionEvent e) {
    LOGGER.info("fireTreeNodeChanged node='" + this + "'");
    synchronized (OBJ_LOCK) {
        for (ActionListener listener : listeners) {
            listener.actionPerformed(e);
        }
    }
}

This code 'leaks' the lock on the listeners. While you are calling the actionPerformed(e) events in the listeners you are still locking this Node (in fact, every Node).

You only need to synchronize the access to the listeners data. A simple way to solve this is to:

private void fireNodeChanged(ActionEvent e) {
    LOGGER.info("fireTreeNodeChanged node='" + this + "'");
    ActionListener[] tmpListeners = null;
    synchronized (OBJ_LOCK) {
        tmpListeners = listeners.toArray(new ActionListener[listeners.size()]);
    }
    for (ActionListener listener : tmpListeners) {
        listener.actionPerformed(e);
    }

}

Now there is not a lock-leak.

This type of problem happens in a few places.
WeakHashMap

listeners = Collections
        .newSetFromMap(new WeakHashMap(32, 0.75f));

This is not doing what you think it does.

Because the TreeModelListener instances are the key to the WeakHashMap, they will never be garbage-collected (the key is a strong-reference....).

Using a WeakHashMap is complicated, and harder to describe than what can easily go here.

StackExchange Code Review Q#45449, answer score: 2