Bug 50259 - [40cat] IDE hung when adding java platform
[40cat] IDE hung when adding java platform
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Ant
4.x
PC Linux
: P2 (vote)
: 4.x
Assigned To: Jesse Glick
issues@projects
: THREAD
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-10-12 08:47 UTC by vanob
Modified: 2004-10-18 11:08 UTC (History)
7 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Full thread dump (25.75 KB, text/plain)
2004-10-12 08:50 UTC, vanob
Details
Cleaned up thread dump for your convenience. (18.62 KB, text/plain)
2004-10-12 13:05 UTC, Petr Nejedly
Details
Patch of the PropertyUtilsTest showing the problem (5.82 KB, patch)
2004-10-12 17:12 UTC, Petr Hrebejk
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vanob 2004-10-12 08:47:07 UTC
[ BUILD # : 200408200409 ]
[ JDK VERSION : J2SE 1.5.0 ]

I've got beta 2

I had to platforms in netbeans. jdk 1.5 and jdk
1.4.2_05. I use 1.5 only for running netbeans.'
all my projects use 1.4.2 as the platform.

NetBeans was closed.
I downloaded and installed jdk 1.4.2_06 and
uninstalled 1.4.2_05. 
Started Netbeans.
It complained that there are some problems.
I went to Tools/Platform Manager
and pressed add platform, after that I chose
/usr/java/j2sdk1.4.2_06 folder and pressed enter
and IDE hung.

Attaching thread dump
Comment 1 vanob 2004-10-12 08:50:11 UTC
Created attachment 18216 [details]
Full thread dump
Comment 2 _ ttran 2004-10-12 09:59:25 UTC
Petr N: please investigate and reassign
Comment 3 Petr Nejedly 2004-10-12 11:04:06 UTC
The build is quite old, anyway, the deadlock is simple:
In AWT, PlatformConvertor puts global properties, which writes to a
file under PM.mutex(write). It causes a file event, processed by web
probject which in turn ends up waiting on Children.mutex(write)

In one RP thread, there is a BrokenLinks action running, holding a
Children.mutex(write) and trying to get PM.mutex(read).

Passing to j2seproject for further evaluation.

Note: the thread dump also reveals one interresting thing:
two BrokenLinksActions running concurently, as the lock
they synchronize on is per action instance and there are two such
instances. Not sure if it is OK or not, just looks suspicious.
Comment 4 Petr Nejedly 2004-10-12 13:05:43 UTC
Created attachment 18225 [details]
Cleaned up thread dump for your convenience.
Comment 5 Tomas Zezula 2004-10-12 13:38:00 UTC
org.netbeans.modules.project.ui.ProjectsRootNode$ProjectChildren.stateChanged(ProjectsRootNode.java:279)
calls org.openide.nodes.Children$Keys.refreshKey(Children.java:1816).
It is not good to acquire lock in the listener.
The refresh should be rescheduled.
Comment 6 Petr Hrebejk 2004-10-12 14:39:38 UTC
Checking in
projects/projectui/src/org/netbeans/modules/project/ui/ProjectsRootNode.java;
/cvs/projects/projectui/src/org/netbeans/modules/project/ui/ProjectsRootNode.java,v
 <--  ProjectsRootNode.java
new revision: 1.31; previous revision: 1.30
done
Comment 7 Petr Hrebejk 2004-10-12 15:30:10 UTC
Jesse, you might want to look at this as iut looks like I get the
event under some lock and the fix should rather be called hotfix.
Comment 8 Petr Hrebejk 2004-10-12 17:09:34 UTC
Reopening, it seems that the
org.netbeans.spi.project.support.ant.ProjectUtils does not behave
correctly. Will attach test (made by Jarda) which proves that.
Comment 9 Petr Hrebejk 2004-10-12 17:12:20 UTC
Created attachment 18230 [details]
Patch of the PropertyUtilsTest showing the problem
Comment 10 Petr Hrebejk 2004-10-12 17:14:51 UTC
Consider removing the hotfix in ProjectsRootNode when fixed.

Comment 11 Jesse Glick 2004-10-12 18:30:49 UTC
Sorry, I don't consider the "hotfix" to be anything more than a proper
fix, and I don't agree with the proposed tests, insofar as I
understand what they are trying to test:

1. testNoFiringUnderWriteAccess - no way. The file change is fired
when it happens.

2. testFiringInRightThread - property changes are coalesced and fired
in read access. Firing them immediately is way too expensive for
performance. If you happen to ask for some value before the coalesced
changes are fired, then changes are fired right then.

ProjectsRootNode$ProjectChildren.stateChanged cannot assume what locks
are being held when it is called, and so it is not permitted to
acquire a new lock (Children.MUTEX). Once Children threading is fixed
to make it behave like a regular GUI component, this will not be an
issue since there will be no Children lock at all.
Comment 12 Jaroslav Tulach 2004-10-13 10:17:09 UTC
It would be very valuable to know what is the real intention of using
Mutex in PropertyUtils. Without that, one can only guess whether the
bug is in the idea or in the implementation.

After reading the code I think that the intention is to do few
modifications in batch under write access and notify other code then,
under read access. This is creditable effort, however as shown by the
tests, it is not implemented correctly.

The more important test is #2, which shows that the changes can be
fired sooner than the batch is finished and the events are delivered
without read access. This is inconsistent with the usual case when
listeners are notified at the end of writeAccess by postedReadRequests.

The case #1 is a bit patological, but shows actual problem and can be
easily fixed by using AtomicAction. I would suggest that as well.

Re. ProjectsRootNode$ProjectChildren - they are innocent (as shown by
the tests). The flaw is in PropertyUtils because they are calling
foreign code while some thread is holding write lock.
Comment 13 Jesse Glick 2004-10-13 18:22:46 UTC
First of all, the apparent bug was apparently fixed, so please don't
reopen this unless the bug reoccurs. If you have some other issue,
open a fresh bug.

Re. #1, I still see no problem. You store a new .properties file, you
get a file change event right then. As designed.

Re. #2, perhaps I don't see what your test is trying to demonstrate,
but I don't see anything wrong. Property changes are coalesced (since
it is too costly to fire them individually) and normally notified
after a block of changes in the source providers. In fact, it is not
the property changes themselves that are too expensive, it is
recomputing the list of properties, so this is deferred as long as
possible in case more source changes come in. If however someone needs
a property before the scheduled recomputation and change firing, then
the current map is computed immediately.

I do not agree that the previous version of ProjectChildren was
"innocent". The current Children implementation acquires a lock when
you call setKeys. This is unfortunate, and I think in the future it
should be changed to run later in EQ which would make it safe, but for
now it does. Therefore you cannot call setKeys safely when there might
be some other lock held - e.g. while you are in a listener callback.

Re. "The flaw is in PropertyUtils because they are calling foreign
code while some thread is holding write lock." - what foreign code are
you referring to?

Again, if you still feel there is something wrong, open a (lower
priority) bug separately with more information.
Comment 14 David Konecny 2004-10-14 12:31:00 UTC
*** Issue 50355 has been marked as a duplicate of this issue. ***
Comment 15 David Konecny 2004-10-14 14:36:58 UTC
"Re. #1" - I agree with Jesse. I think there are two layers: FS layer
and Project layer. Anybody interested in project properties should
listen on Project layer and not respond to FS events so it does not
matter under which lock the FS events are fired.

"Re. #2" - it seems to me strange that getProperty() call can result
in change events being fired. All I expect is to get a value - I did
not change anything! However, once explained it starts making sense.
It is probably not possible, but could be the events (fired in this
case) be fired under Mutex downgraded to readAccess? I think that's
the state Yarda is asking for and that would make sense to me too.
Comment 16 Jesse Glick 2004-10-14 18:22:38 UTC
Re. #2, "could be the events (fired in this case) be fired under Mutex
downgraded to readAccess?" - perhaps. Depends on whether you care
about the invariant that code which listens for events and invalidates
caches, etc., always "sees" change events "before" seeing new data via
direct query. This is sometimes important - maintaining chronological
accuracy - and sometimes not. Firing change events later would make
such clients incorrect; i.e. if your code is written to assume that
the state of the object (property evaluator, here) will not have
changed if you have not gotten change events from it, then your code
will be wrong. Of course there is a compromise; you can choose from

1. Fire events eagerly. Safe, but sometimes (as here) slow to compute.

2. Fire events lazily, but immediately if state is accessed directly.
The current impl. Safe, though has the odd property that calling a
non-mutating method can trigger delivery of a change event, as you
observed.

3. Fire events lazily and asynchronously (i.e. either a different
thread, or using something like readAccess which uses the same thread
but at an effectively unpredictable future point). Efficient, and no
events triggered by getters, but can break clients which are not
expecting this semantics.
Comment 17 Jaroslav Tulach 2004-10-15 07:25:12 UTC
Sorry, I may be slow, but I do not see answer to my initial question:
What is the (supposed) threading model of this class? 

I am reopening, because whatever the goal is, it is definitively not
achieved. I am looking forward to share the vision of the desired state.

Comment 18 David Konecny 2004-10-15 15:27:40 UTC
Re. "1. Fire events eagerly." - was it measured how slow this really
is? It the gain worth the trouble?
Comment 19 Jesse Glick 2004-10-15 20:12:06 UTC
Again, this issue is a bug report about a deadlock; for any separate
questions it may have inspired please use a separate issue.

Again, the intended threading model for the property evaluator is none
- it should never be accessed concurrently from more than one thread,
so use a mutex if in doubt. That holds for most of the project system
metadata code.

Re. speed of firing events eagerly - yes, this was the original state,
until maybe a month ago. It showed up as being quite slow in a
profiler for common user actions. I changed it to the current state
and it was no longer a significant time consumer in the profiler.
(Each recalculation of a PropertyEvaluator is nontrivial operation,
since you normally have dozens of properties, many of them inherited
from System.getProperties, and they can refer to one another
recursively etc. - lots of string processing. It could perhaps be sped
up using more sophisticated algorithms, but I do not know how to do so
currently.)
Comment 20 Jaroslav Tulach 2004-10-18 11:08:51 UTC
I do not agree that this deadlock has been fixed as the root cause is
still there, but as you requested that issue 50546 which talks just
about the root cause has been created.



By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo