This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 50546 - Inconsistent (and broken) threading strategy in PropertiesUtils
Summary: Inconsistent (and broken) threading strategy in PropertiesUtils
Status: RESOLVED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Ant (show other bugs)
Version: 4.x
Hardware: PC Linux
: P2 blocker (vote)
Assignee: Jesse Glick
URL:
Keywords: THREAD
Depends on:
Blocks:
 
Reported: 2004-10-18 11:05 UTC by Jaroslav Tulach
Modified: 2004-10-26 19:45 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2004-10-18 11:05:15 UTC
As requested by jglick@netbeans.org in issue 50259
creating separate issue for the problem described
there: The PropertiesUtils operate under shared
global lock and call 3rd party code with/without
the lock depending on random conditions. This is
unexpected and causes deadlocks.

The main error is that by default all listeners
are notified at the end of batch updated under
readAccess, while sometimes listeners can be
notified without readAccess, while other thread is
holding write access!!!

The problem is demostrated by two tests
http://www.netbeans.org/issues/showattachment.cgi?attach_id=18230
Comment 1 Jesse Glick 2004-10-18 20:57:39 UTC
As previously mentioned, I don't think I agree with the suggested unit
tests, and don't know of any actual harm caused by the current
condition. Nonetheless I would like to put more safety guards on this
code just in case someone is calling it incorrectly (e.g. from
multiple threads). Not a high priority for me but should be fixed.
Comment 2 Jesse Glick 2004-10-18 21:23:50 UTC
Yarda suggests

assert ProjectManager.mutex().isReadAccess &&
!ProjectManager.mutex().isWriteAccess();

in all PropertyEvaluator callbacks, which is reasonable.

Don't exactly need so many people on CC, I think (readd yourself if
interested).
Comment 3 Jaroslav Tulach 2004-10-19 16:01:29 UTC
Thanks for thinking the assert is reasonable. Please notice that if
implemented, the second unit test will start to pass. As that is
exactly what the tests simulates.
Comment 4 Jesse Glick 2004-10-19 16:19:49 UTC
The second test seems to assert that the PE change notifications will
be delivered in the same thread as the thread that wrote the
*.properties file. Which I do not guarantee - in particular, not in
the (odd) situation shown in the test.
Comment 5 Jaroslav Tulach 2004-10-19 17:06:30 UTC
Try to deliver changes under isReadAccess when the other thread is
under isWriteAccess. By fixing the assertion you fix this test as well.
Comment 6 Jesse Glick 2004-10-19 17:56:15 UTC
I don't think so (haven't tried yet) - T1 acquires W, writes
something.properties, releases W, then T2 acquires R, asks for a
value, receives a change, releases R.
Comment 7 Jaroslav Tulach 2004-10-20 08:44:27 UTC
You are right, this might happen (after recent pnejedly fix in Mutex).
I guess this is fine, the biggest problem I could see was that one
thread is notified while the other is in W. Which would be prevented
by requiring R to notify listeners.
Comment 8 Jesse Glick 2004-10-20 19:49:10 UTC
Right, what you mention should be prevented.
Comment 9 Jesse Glick 2004-10-26 03:40:08 UTC
I played around with different strategies but eventually decided to
just fire the changes synchronously.

The main reason to defer them was that j2seproject's
ProjectClassPathImplementation was refiring changes whenever the
evaluator changed - see issue #47910. Turns out that when *adding* a
lot of JARs to the classpath (e.g. 100), this is not a problem - I
guess the references are added first, then javac.classpath (etc.) is
changed. But when *deleting* a lot of JARs, the references are removed
one by one, firing changes in javac.classpath each time, and this was
causing massive overhead.

If I just change PCPI to coalesce change firing in the classpath, then
it seems to be fine. Since I don't know of any particular reason why
PCPI has to fire changes synchronously (other than potential unit
tests - not yet written, apparently), this seems preferable, since it
moves the asynch quality to a higher layer where it is less likely to
cause problems.

Pavel B. - web/project's PCPI should perhaps be changed analogously.
Comment 10 Jesse Glick 2004-10-26 19:45:03 UTC
A bunch of changes:

committed   * Up-To-Date  1.35       
ant/freeform/src/org/netbeans/modules/ant/freeform/FreeformProjectGenerator.java
committed   * Up-To-Date  1.8        
ant/freeform/src/org/netbeans/modules/ant/freeform/ui/ProjectModel.java
committed   * Up-To-Date  1.8         ant/project/nbproject/project.xml
committed   * Up-To-Date  1.25       
ant/project/src/org/netbeans/spi/project/support/ant/AntProjectHelper.java
committed   * Up-To-Date  1.11       
ant/project/src/org/netbeans/spi/project/support/ant/ProjectProperties.java
committed   * Up-To-Date  1.26       
ant/project/src/org/netbeans/spi/project/support/ant/PropertyUtils.java
committed   * Up-To-Date  1.20       
ant/project/src/org/netbeans/spi/project/support/ant/ReferenceHelper.java
committed   * Up-To-Date  1.16       
ant/project/test/unit/src/org/netbeans/spi/project/support/ant/PropertyUtilsTest.java
committed   * Up-To-Date  1.6        
java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/ProjectClassPathImplementation.java