Created attachment 4618 [details]
full thread dump

Created attachment 4619 [details]
full thread dump

Lowering priority to P3, since it's not reproducible.

Probably ErrorManager.getDefault should not block on the folder
recognizer thread (i.e. lookup); it is called from too many different
places for this to be safe.

David, DEADLOCK, so it's yours :-)

Changing ErrorManager.getDeafult() ...

*** Issue 20365 has been marked as a duplicate of this issue. ***

Created attachment 4657 [details]
Proposed patch for ErrorManager

Anyone willing to review my proposed patch? Thanks in advance - I need
at least one person to tell me that what I am doing is not completely
insane.

Please note that I propose to return simple() if two threads are
calling getDefault simultaneously. Also only the first thread calling
getDefault will call lookup. Not 100% deadlock proof but should help.
Another idea is to reschedule the initial lookup call to a thread that
would be known  (and meanwhile keep returning simple() before this
dedicated thread assigns a value to current).

Any suggestions how to do better?

Patch looks OK to me. In principle it would be safest to not block on
lookup even on the first call, but in practice I doubt this matters.
ErrorManager.getDefault is called early, and NbTopManager.Lkp
initially hardcodes it to be NbErrorManager or
${org.openide.ErrorManager}, meaning lookup on it should not block on
anything important.

The patch is not perfect - somebody can get the simple()
implementation even errormanager is in lookup. I suggest to write a
delegating ErrorManager impl instead of the Simple one that will
obtain the list of EMs and delegate to them (also might solve SIM
module problem). If no found it will behave as our current simple().

The biggest problem will be with EM.getInstance (...) it needs to be
properly delegated and updated when list of EMs changes...

In other words:

1. ErrorManager.getDefault() *always* returns a special delegating EM
inside ErrorManager.java. getInstance also creates a delegating EM.

2. Core continues to supply NbErrorManager in lookup and other modules
may add other ErrorManager impls in lookup.

3. ErrorManager asynchronously finds the set of ErrorManager impls in
lookup and listens to changes in it.

4. Any call to the delegating EM forwards the call to the first EM in
the current lookup result.

Still does not solve SIM's problem of needing to add some behavior to
the core NbEM without overriding all of it. That I think requires an
API addition - something like ErrorManager.{Event,Listener}.

Yarda suggested:
Instead of your 4. use this one:

4. Any call to the delegating EM forwards the call to *all* EMs in
the current lookup result.

This should solve it, right?

Maybe, but it seems questionable. What if there are several EM's
registered all of which implement notify() in a way which would
display something to the user? or log()? If it is NbErrorManager and
the SIM EM impl, I think notify() is OK, but it is not clear how well
this would work for e.g. NbErrorManager and TraceLogger.

The Java Event model seems to me better suited to this sort of thing:
you can use event.isConsumed() to decide whether to continue passing
along the event to another consumer.

Created attachment 4664 [details]
New patch - delegating error manager.

I have tried the delegating approach. I beleive we can go on with
delegating without listening. The interested module can always hide
the default implementation if they clash. I can imagine two kinds of
ErrorManagers - silent (logging) ones and those taking care of
everything (not needing the default one).

Anyone willing to check my sources? The delegating approach has to
deal with getInstance and updating the results. I will test it more
before integrating.

I think that looks right, though findAnnotations should probably merge
results.

getInstance & setDelegates can collide due to no synchronization.
delegates do not keep original order.

otherwise ok - if we ignore the need of delegation for Annotation,
that is not needed right now, but can cause ClassCastEx. in future...

What ClassCastException? It is a bug in an ErrorManager impl if it
assumes that it created all the attachments on a throwable. It should
check with instanceof if it requires info from a private impl class.

Fixing the synchronization. Will post another patch soon.
As to order - allInstances returns a Collection. Should it be
SortedSet? Or JavaDoc for allInstances() improved?

Created attachment 4691 [details]
Patch with synchronization fixed

allInstances may return a Collection, but in typical usage the order
will be significant. In other cases it might not be. So I think
Collection is appropriate. SortedSet implies it is ordered by some
comparator, which is not the case. List implies that it is
*necessarily* ordered, which is also not the case.

The patch applied in the main trunk.
ErrorManager 1.14.

*** Issue 20594 has been marked as a duplicate of this issue. ***

Cannot reproduce.

Resolved for 3.4.x or earlier, no new info since then -> closing.