Please use the Apache issue tracking system for new NetBeans issues (https://issues.apache.org/jira/projects/NETBEANS0/issues) !!
Bug 146072 - Incompatible change of ProjectManager.mutex
Incompatible change of ProjectManager.mutex
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Infrastructure
6.x
All All
: P2 (vote)
: 6.x
Assigned To: Milan Kubec
issues@projects
:
Depends on: 146525 147492
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-03 17:09 UTC by Tomas Zezula
Modified: 2008-09-23 18:11 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Rough fix of Tomáš's test (10.85 KB, patch)
2008-09-04 17:27 UTC, Jaroslav Tulach
Details | Diff
If the original test used EditableProperties, it would pass now (32.09 KB, patch)
2008-09-06 22:50 UTC, Jaroslav Tulach
Details | Diff
Diff ready for pre-integration review (37.80 KB, patch)
2008-09-08 16:12 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Zezula 2008-09-03 17:09:15 UTC
The behavior of ProjectManager.mutex has incompatible changed causing j2seproject tests to fail. For now I put workaround to let at least the tests pass.
The problem is that file changes caused in the project mutex critical section are not visible to the code in the same critical section (missing events).
Comment 1 Jaroslav Tulach 2008-09-04 17:27:50 UTC
Created attachment 69081 [details]
Rough fix of Tomáš's test
Comment 2 Jaroslav Tulach 2008-09-06 22:50:14 UTC
Created attachment 69236 [details]
If the original test used EditableProperties, it would pass now
Comment 3 Jaroslav Tulach 2008-09-06 23:00:20 UTC
I guess now is the time for you to add few comments about ugliness... I do not want to compromise your code, but if I 
am supposed to propose a low risk fix at this stage of development, which does not change anything, just makes the 
Tomáš's test pass, the change would look like 
http://www.netbeans.org/nonav/issues/showattachment.cgi/69236/EditablePropertiesDeliverEventsImmediatelly.diff
It delivers events synchronously (without use of RequestProcessor), plus if one uses EditableProperties.store(fo), it 
delivers them immediately. This makes Tomáš's test pass without adding any ugliness on the side of his API usage. The 
implementation requires review from people who know how things are supposed to work. It adds one desirable API change 
and one unwanted, yet needed one. There are some tests, more would be beneficial. That is the whole prototype.
Comment 4 Jaroslav Tulach 2008-09-08 16:12:39 UTC
Created attachment 69326 [details]
Diff ready for pre-integration review
Comment 5 Jaroslav Tulach 2008-09-08 16:16:18 UTC
The last diff
http://www.netbeans.org/nonav/issues/showattachment.cgi/69326/EditablePropertiesDeliverEventsImmediatelly.diff
is documented enough to be ready for API review, the tests of all three affected modules are OK (except issue 146525, 
but that is unrelated to this change) and the change seeks minimal influence to 6.1 behaviour. I guess it is up to 
Milan to get comfortable with the change and send it to API review.
Comment 6 Jaroslav Tulach 2008-09-09 08:59:04 UTC
Milan feels it is OK to get comments from everyone.
Comment 7 Milan Kubec 2008-09-09 09:13:25 UTC
Yes, please.
Comment 8 Tomas Zezula 2008-09-09 10:16:31 UTC
The patch looks fine. I only want to point out that it may cause some problems in code depending on non coming events in writeAccess, but this is expected 
and I doubt there are any.
I would prefer the method isMutexEvent to be in SPI/support since it is not probably first order API and looks strange.
Comment 9 Jesse Glick 2008-09-10 05:28:16 UTC
I still consider all this a workaround for a bug in ProjectManager, but it will have to do for now. Would rather not
have isMutexEvent document that PM is obligated to delay delivery of FS events. BTW typo: 'genereated'


[JG01] What is the purpose of the patch to J2SEConfigurationProvider.java? It just looks to rename and move a method for
no reason.


[JG02] Please do not add EditableProperties.store(FileObject) to the API. EP is intended to be a pure utility class
similar to java.util.Properties, destined ultimately for some generic utilities module along with EditableManifest, and
a dependency on FileObject is unwelcome. The dependency on FileChangeSupport is certainly not acceptable. You can add
this method to org.netbeans.modules.project.ant.Util (this will be visible to unit tests so no general API addition is
needed). Anyway the body is too long and complicated; you maybe copied it from some old clumsy code. It need only do

OutputStream os = fo.getOutputStream();
try {
    ep.store(os);
} finally {
    os.close();
}


[JG03] Why is apichanges talking about AntProjectHelper.createSimpleAntArtifact?


[JG04] Using System.currentTimeMillis() to implement a sequence number is a bad idea; you do not know what the clock
tick is. Use an AtomicInteger if you need a logical timestamp.


[JG05] Please revert the apparently bogus indentation changes in GeneratedFilesHelper.java.


[JG06] Delete the suite() method from FileChangeSupportTest.java. Pester mpetras to implement run-single-method in
apisupport.project.


[JG07] Mark testDirectNotifyChanges and testDirectNotifyChangesInAtomicAction @RandomlyFails if you are going to use SLEEP.
Comment 10 Jan Lahoda 2008-09-10 12:59:50 UTC
JL01: what is the inevitable reason why we need PM.mutex() atomic-lock filesystems? This does not seem right to me. To
my knowledge, the project does not have to store its metadata on FileSystems. Moreover, it seems to me that we are
atomic-locking the FileSystems even for readlocks - why? Wouldn't it be more correct to simply not atomic-lock the
FileSystems for PM.mutex()?
Comment 11 Milos Kleint 2008-09-10 13:29:18 UTC
the issue seems highly controversial and does not warrant the FAST track review
1. the current "solution" introduces rather incomprehensible concept that will stick with us for a few releases (read
forever)
2. the problem was caused by as workaround for a problem that was caused by another controversial problem
(DataObject.getLookup(). 

Personally I don't think the mutex wrapping by atomic action is correct in general and should be rollback. Therefore the
current problem disappears (am I right?) and there's no need to introduce this api change.

Comment 12 Andrei Badea 2008-09-10 13:44:05 UTC
This introduces an unwanted change in an attempt to defend another unwanted change. It does not contribute to the
clarity or ease of use of our API's -- quite the opposite, actually -- so I do no agree with it.

I think a better solution would be to remove the AtomicAction wrapping hack. I almost have the list of DataObject's that
need to be fixed in order to avoid the deadlocks the AA hack tried to avoid, and I can fix them.

Anyway, in case the change is accepted:

[AB01] Would too prefer that isMutexEvent() not document the AA hack, since that would make it harder to remove the hack.

[AB02] Why is mkubec mentioned as the author of the changes?
Comment 13 Milan Kubec 2008-09-17 14:22:59 UTC
Lowering priority to P2, since there are no user visible issue caused by the change so far. We will try to fix it by
reverting the runAtomicAction part of the PM.mutex along with other fixes in DataObjects.
Comment 14 Milan Kubec 2008-09-19 14:27:01 UTC
Fixed. runAtomicAction part was removed from mutex.

http://hg.netbeans.org/main/rev/9839724a6882
Comment 15 Quality Engineering 2008-09-20 05:47:15 UTC
Integrated into 'main-golden', will be available in build *200809200201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/9839724a6882
User: Milan Kubec <mkubec@netbeans.org>
Log: #146072: reverting runAtomicAction part of the fix for issue #91291; might cause undefined behavior
Comment 16 Quality Engineering 2008-09-21 05:12:24 UTC
Integrated into 'main-golden', will be available in build *200809210201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/799aec2222cd
User: Milan Kubec <mkubec@netbeans.org>
Log: #146072: had to revert to previous version of PM.mutex, some tests are failing
Comment 17 Milan Kubec 2008-09-22 08:10:39 UTC
Reopening the issue, some tests are failing.
Comment 18 Milan Kubec 2008-09-22 14:24:44 UTC
Fixed again, now with tests.

http://hg.netbeans.org/main/rev/72931bc4f209
Comment 19 Quality Engineering 2008-09-23 18:11:55 UTC
Integrated into 'main-golden', will be available in build *200809231435* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/72931bc4f209
User: Milan Kubec <mkubec@netbeans.org>
Log: #146072: PM.mutex won't run inside runAtomicAction; tests fixed


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