Bug 172012 - TopComponent.getRegistry().getOpened() not safe to iterate
TopComponent.getRegistry().getOpened() not safe to iterate
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Window System
6.x
All All
: P2 (vote)
: 6.x
Assigned To: Jaroslav Tulach
issues@platform
: THREAD
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-11 10:30 UTC by Jaroslav Tulach
Modified: 2009-09-18 22:23 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Implementation that is both iterable and live (10.98 KB, patch)
2009-09-16 12:49 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2009-09-11 10:30:19 UTC
Natural code like

        for (TopComponent tc : TopComponent.getRegistry().getOpened()) {
            final EditorCookie ec = tc.getLookup().lookup(EditorCookie.class);
            if (ec != null) {
                ec.close();
            }
        }

can cause failures like the one in
http://deadlock.netbeans.org/hudson/job/NB-Core-Build/3230/testReport/org.netbeans.test.ide/MemoryValidationTest/testGCDocuments/

java.util.ConcurrentModificationException
        at org.openide.util.WeakSet$WeakSetIterator.checkModcount(WeakSet.java:481)
        at org.openide.util.WeakSet$WeakSetIterator.hasNext(WeakSet.java:432)
        at java.util.Collections$UnmodifiableCollection$1.hasNext(Collections.java:1009)
        at org.netbeans.test.ide.WatchProjects.assertTextDocuments(WatchProjects.java:157)
        at org.netbeans.test.ide.IDEValidation.testGCDocuments(IDEValidation.java:1407)
        at org.netbeans.jellytools.JellyTestCase.runTest(JellyTestCase.java:180)
        at org.netbeans.junit.NbTestCase.access$200(NbTestCase.java:88)
        at org.netbeans.junit.NbTestCase$2.doSomething(NbTestCase.java:336)
        at org.netbeans.junit.NbTestCase$1Guard.run(NbTestCase.java:273)
        at org.netbeans.junit.NbTestCase.runBare(NbTestCase.java:355)
        at org.netbeans.jellytools.JellyTestCase.runBare(JellyTestCase.java:147)
        at org.netbeans.junit.NbTestCase.run(NbTestCase.java:213)
        at org.netbeans.test.ide.MemoryValidationTest.run(MemoryValidationTest.java:65)
        at org.netbeans.junit.NbModuleSuite$S.runInRuntimeContainer(NbModuleSuite.java:784)
        at org.netbeans.junit.NbModuleSuite$S.run(NbModuleSuite.java:572)
Comment 1 Jesse Glick 2009-09-11 14:30:22 UTC
P1 since it is blocking core-main propagation.

Starting failing in a build in which I pushed project system changes; don't see offhand how that could be related.
Comment 2 Jaroslav Tulach 2009-09-11 14:52:25 UTC
It may not be related to threading. The code:

for (TopComponent tc : TopComponent.getRegistry().getOpened()) {
  tc.close();
}

is likely to fail in case there are at least two top components. Easy to write unit test for it, I am sure. The reason 
why the test was passing before is that Jelly managed to close all editors before my code run. Is not the test broken 
by some other changes that prevent documents to be closed?
Comment 3 Stanislav Aubrecht 2009-09-11 15:02:35 UTC
well, i can make TopComponent.Registry.getOpened() safe to iterate but the javadoc says:

@return live read-only set of {@link TopComponent}s

so this fix is likely to break some other functionality which depends on the set being live.
Comment 4 Jesse Glick 2009-09-11 15:18:39 UTC
I think IDEValidation is failing due to some bundle issues which I will fix. The two usages of Registry.getOpened in
WatchProjects are nonetheless still probably wrong.
Comment 5 Quality Engineering 2009-09-13 21:06:37 UTC
Integrated into 'main-golden', will be available in build *200909131354* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/de67067bd697
User: Jesse Glick <jglick@netbeans.org>
Log: #172012: do not iterate over a set (TC.Registry.opened) whose elements might be being deleted.
Comment 6 Jaroslav Tulach 2009-09-16 12:46:15 UTC
I still suggest this is fixed. It is not that strange code to write. Also the implementation shall be made consistent 
between the two implementations we have. The default one in openide.windows and the real one in core.windows.
Comment 7 Jaroslav Tulach 2009-09-16 12:49:04 UTC
Created attachment 87758 [details]
Implementation that is both iterable and live
Comment 8 Stanislav Aubrecht 2009-09-16 14:06:48 UTC
go ahead and integrate the patch, it looks fine to me

(or it might be easier to introduce method Set<TopComponent> getOpenedSnapshot() to provide a copied set of
topcomponents that were opened at the time of the calling of the method)
Comment 9 Jaroslav Tulach 2009-09-16 16:21:38 UTC
core-main#6608ef539ffd
Comment 10 Jaroslav Tulach 2009-09-17 08:10:39 UTC
Btw. the TC.Registry is (mistakenly) a Java interface, so adding methods to it is not something I'd like to advocate. 
Thus I have chosen to improve the implementation of existing method.
Comment 11 Quality Engineering 2009-09-18 22:23:08 UTC
Integrated into 'main-golden', will be available in build *200909181401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/6608ef539ffd
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #172012: Live and safely iterable Set from getOpened() method


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