Bug 36424

Created attachment 11785 [details]
Proposed fix

Don't really like the patch - potentially prone to deadlocks.

In the branch, all changes are fired in EQ. Only one piece was merged
to the trunk (I needed it in trunk for some reason). It is intentional.

I will try to solve the problems you have (just in unit tests AFAIK)
in another way.

What about the difference between fireOwnPC and firePC? They should in
my opinion follow the same threading model.

Another thing: In your thread documents you consider asynchronous
firing one of the biggest evils. Why introduce such behaviour as new
feature? The proper thing in my opinion is to disallow firing outside
of AWT and fix all places that do that. The current state will just
introduce asynch problems as in case of Hrebejk's tests.

Btw. Hrebejk's tests are not the only ones that are failing. I suggest
to put into the Mutex.EVENT runnable Thread.sleep (1000) and see what
we happen, as I had continuous failures in SheetTest,
FilterChildrenEventsTest, SetChildrenTest and NodeLookupTest which
disappeared immediatelly when I switched into synchronous firing.

Re. fOPC and fPC - yes, of course, and in the branch they do. Just
only needed one change in the trunk.

Re. introducing asynch firing - right, I *don't* recommend it, but it
seemed that there are so many places in modules that do in fact
already try to fire Node changes from off EQ that trying to fix them
all would be a huge amount of work, and would be a major and
unpleasant incompatibility for third-party modules. (IMHO requiring
changes in a few obscure situations is probably acceptable, but not in
code idioms that almost everyone uses.) While working on the branch I
did in fact start by trying to require that all changes be fired from
EQ, but soon found so many assertion errors that it seemed impractical
to try to correct them all, so I changed course. Instead I am trying
to permit a set of commonly used methods, frequently called from
various threads, e.g. setKeys or Node.fPC, to be called safely from
any thread for compatibility.

Anyway the principal danger of asynch firing is from a data model,
since clients may be expecting the change to come promptly. In the
case of Nodes, almost all the listeners are going to be coming from
one smallish body of code - Explorer & Property Sheet - which I think
doesn't care exactly when the changes are fired.

Re. Hrebejk's tests - my current plan (untested) is to force EQ-only
behavior only for nodes not added to some visual tree. Before a node
(or subtree) is displayed, it can be manipulated from any thread - but
only that thread, i.e. not thread-safe. Still TBD whether I can detect
the visualization reliably enough to make this work. If so, then
changes would be fired synch unless the node is visualized *and* the
call comes from off EQ - thus unit tests running off EQ with
unvisualized nodes would still see synch behavior.

Alternately, just use EQ.iAW from the unit tests. A little ugly but
should work.

I will try to check other unit tests as well. One thing I have noticed
is that XTest does not seem to run code with -ea, which would mean
that the tests aren't running in as strict a mode as e.g. dev builds
are, which is clearly wrong.

I am still searching for a bit compatible solution. One idea might be
in adding:

void addNodeListener (NodeListener l, boolean notifyInAWTorAnywhere)

that would satisfy the compatibility, it would make the explorer code
simpler and smaller and would not introduce memory or performance
overhead. 

Btw. what was the reason for merging just one change from your branch
and not everything?

Btw. firing of add/remove nodes should be done in the same way as
other firing as well. But here I guess you are not able to easily
guarantee asynchronicity.

Re. iAWT for all tests - that probably influence a lot of tests. Just
try the notification with some bigger delay.

Re. "realized/not realized nodes" a bit hard to decide. There is no
way to capture all calls to nodes. Maybe getChildren trick? The
AWT/non AWT listener seems more easy.

Re. special addNodeListener - will consider it. Ugly to have in the
API though; better (IMHO) to move towards the desired API. Of course I
would like to make Explorer code simpler and smaller, keep
compatibility for most client code, and not introduce overhead - most
of this is (I think) already true in the branch.

Reason for merging just that change - sorry, don't remember exactly
know, but I think it had to do with action presenters; there were some
errors in the trunk for a day or two which that fixed. Can try
reverting it, if you need the synch firing urgently. However I think
EQ.iAW for the firing is rather dangerous.

Re. firing add/remove nodes - in the branch, this is always in EQ;
most Children methods must be called in EQ; C.K.setKeys uses
Mutex.EVENT.writeAccess(Runnable), for compatibility.

"Re. iAWT for all tests - that probably influence a lot of tests. Just
try the notification with some bigger delay." - don't understand what
you are saying...? (iAW = invokeAndWait, iAWT a typo?)

Re. trapping first call to getChildren() - probably won't work, as
some (bad) code using Children.Array and not using addNotify would
trigger this, and some (bad) code storing extra state in a Children
subclass might too, and FilterNode calls it too. Code trying to call
getChildren().getNodes() off EQ is another issue - I already fixed as
many of these as I could find in trunk, since it seems rare and I
cannot think of any way to make it safe. (Generally only really poorly
written module code would call this anyway. I saw a couple of
examples, and it was not pretty.)

Probably can just have Explorer views notify root nodes that they are
to be displayed in a view. This seems pretty simple. I will try it.

"Re. iAWT for all tests - that probably influence a lot of tests. Just
try the notification with some bigger delay." - Sorry, better is:
invokeLater probably influence a lot of tests, just add Thread.sleep
(1000) as first line of run() and see how much tests will fail.

Re. revert - something should be done to fix all tests, as right now
our unit tests are useless and I rely on them heavilly. So does
Hrebejk, I think.

Second option to add*Listener (listener, boolean) is to have marker
interface EventQueueListener { } to find out whether the NodeListener
and PropertyChangeListeners should be in AWT (if implement the marker)
or not.

Rolling back change in the trunk, but putting in a similar change for
DataNode (should not hurt most unit tests). Otherwise you can get
deadlocks; attaching an example.

committed   * Up-To-Date  1.7        
openide/loaders/src/org/openide/loaders/DataNode.java
committed   * Up-To-Date  1.78       
openide/src/org/openide/nodes/Node.java

Created attachment 11847 [details]
Sample deadlock occurring in unit tests if the Node patch is reverted on its own

closed