Bug 146072

Created attachment 69081 [details]
Rough fix of Tomáš's test

Created attachment 69236 [details]
If the original test used EditableProperties, it would pass now

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.

Created attachment 69326 [details]
Diff ready for pre-integration review

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.

Milan feels it is OK to get comments from everyone.

Yes, please.

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.

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.

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()?

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.

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?

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.

Fixed. runAtomicAction part was removed from mutex.

http://hg.netbeans.org/main/rev/9839724a6882

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

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

Reopening the issue, some tests are failing.

Fixed again, now with tests.

http://hg.netbeans.org/main/rev/72931bc4f209

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