Index: vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java =================================================================== --- vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java (revision 581320) +++ vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java (working copy) @@ -274,6 +274,9 @@ if (threadGroup == null) { threadGroup = currentThread.group; } + + threadGroup.checkGroup(); + this.group = threadGroup; this.daemon = currentThread.daemon; this.contextClassLoader = currentThread.contextClassLoader; @@ -302,8 +305,6 @@ SecurityUtils.putContext(this, AccessController.getContext()); checkAccess(); - // adding the thread to the thread group should be the last action - threadGroup.add(this); } /** @@ -756,6 +757,9 @@ "This thread was already started!"); } + // adding the thread to the thread group + group.add(this); + if (VMThreadManager.start(this, stackSize, daemon, priority) != 0) { throw new OutOfMemoryError("Failed to create new thread"); Index: vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java =================================================================== --- vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java (revision 581320) +++ vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java (working copy) @@ -25,8 +25,6 @@ import java.util.ArrayList; import java.util.LinkedList; import java.util.Iterator; -import java.util.WeakHashMap; -import java.util.ConcurrentModificationException; /** * @com.intel.drl.spec_ref @@ -83,7 +81,7 @@ /** * All threads in the group. */ - private WeakHashMap threads = new WeakHashMap(); + private LinkedList threads = new LinkedList(); /** * @com.intel.drl.spec_ref @@ -123,24 +121,24 @@ */ public int activeCount() { int count = 0; - List groupsListCopy = null; // a copy of subgroups list - Object[] threadsCopy = null; // a copy of threads list + List groupsCopy = null; // a copy of subgroups list + List threadsCopy = null; // a copy of threads list synchronized (lock) { if (destroyed) { return 0; } - threadsCopy = copyThreads(); - groupsListCopy = (List)groups.clone(); + threadsCopy = (List)threads.clone(); + groupsCopy = (List)groups.clone(); } - for (int i = 0; i < threadsCopy.length; i++) { - if (((Thread) threadsCopy[i]).isAlive()) { + for (Object thread : threadsCopy) { + if (((Thread)thread).isAlive()) { count++; } } - for (Iterator it = groupsListCopy.iterator(); it.hasNext();) { - count += ((ThreadGroup)it.next()).activeCount(); + for (Object group : groupsCopy) { + count += ((ThreadGroup)group).activeCount(); } return count; } @@ -150,16 +148,16 @@ */ public int activeGroupCount() { int count; - List groupsListCopy = null; // a copy of subgroups list + List groupsCopy = null; // a copy of subgroups list synchronized (lock) { if (destroyed) { return 0; } count = groups.size(); - groupsListCopy = (List)groups.clone(); + groupsCopy = (List)groups.clone(); } - for (Iterator it = groupsListCopy.iterator(); it.hasNext();) { - count += ((ThreadGroup)it.next()).activeGroupCount(); + for (Object group : (List)groupsCopy) { + count += ((ThreadGroup)group).activeGroupCount(); } return count; } @@ -193,14 +191,7 @@ throw new IllegalThreadStateException( "The thread group " + name + " is already destroyed!"); } - if (!nonsecureDestroy()) { - throw new IllegalThreadStateException("The thread group " + name + - " is not empty"); - } else { - if (parent != null) { - parent.remove(this); - } - } + nonsecureDestroy(); } } @@ -400,11 +391,23 @@ throw new IllegalThreadStateException( "The thread group is already destroyed!"); } - threads.put(thread, this); + threads.add(thread); } } /** + * Checks that group is not destroyed + */ + void checkGroup() { + synchronized (lock) { + if (destroyed) { + throw new IllegalThreadStateException( + "The thread group is already destroyed!"); + } + } + } + + /** * Removes a thread from this thread group */ void remove(Thread thread) { @@ -438,21 +441,6 @@ } /** - * Copies this ThreadGroup's threads to an array which is used in iteration - * over threads because iteration over a WeakHashMap object can lead to - * ConcurrentModificationException. - */ - private Object[] copyThreads() { - while (true) { - try { - // toArray() can throw ConcurrentModificationException - return threads.keySet().toArray(); - } catch (ConcurrentModificationException e) { - } - } - } - - /** * Used by GetThreadGroupChildren() jvmti function. * @return Object[] array of 2 elements: first - Object[] array of active * child threads; second - Object[] array of child groups. @@ -467,7 +455,7 @@ return new Object[] {null, null}; } - for (Thread thread : threads.keySet()) { + for (Thread thread : threads) { threadsCopy.add(thread); } @@ -503,27 +491,27 @@ * done */ private int enumerate(Thread[] list, int offset, boolean recurse) { - List groupsListCopy = null; // a copy of subgroups list - Object[] threadsCopy = null; // a copy of threads list + List groupsCopy = null; // a copy of subgroups list + List threadsCopy = null; // a copy of threads list synchronized (lock) { if (destroyed) { return offset; } - threadsCopy = copyThreads(); + threadsCopy = (List)threads.clone(); if (recurse) { - groupsListCopy = (List)groups.clone(); + groupsCopy = (List)groups.clone(); } } - for (int i = 0; i < threadsCopy.length; i++) { - if (((Thread) threadsCopy[i]).isAlive()) { - list[offset++] = (Thread) threadsCopy[i]; + for (Object thread : threadsCopy) { + if (((Thread)thread).isAlive()) { + list[offset++] = ((Thread)thread); if (offset == list.length) { return offset; } } } if (recurse) { - for (Iterator it = groupsListCopy.iterator(); offset < list.length + for (Iterator it = groupsCopy.iterator(); offset < list.length && it.hasNext();) { offset = ((ThreadGroup)it.next()).enumerate(list, offset, true); } @@ -551,8 +539,8 @@ } int firstGroupIdx = offset; synchronized (lock) { - for (Iterator it = groups.iterator(); it.hasNext();) { - list[offset++] = (ThreadGroup)it.next(); + for (Object group : groups) { + list[offset++] = (ThreadGroup)group; if (offset == list.length) { return offset; } @@ -575,17 +563,17 @@ private void list(String prefix) { System.out.println(prefix + toString()); prefix += LISTING_INDENT; - List groupsListCopy = null; // a copy of subgroups list - Object[] threadsCopy = null; // a copy of threads list + List groupsCopy = null; // a copy of subgroups list + List threadsCopy = null; // a copy of threads list synchronized (lock) { - threadsCopy = copyThreads(); - groupsListCopy = (List)groups.clone(); + threadsCopy = (List)threads.clone(); + groupsCopy = (List)groups.clone(); } - for (int i = 0; i < threadsCopy.length; i++) { - System.out.println(prefix + (Thread) threadsCopy[i]); + for (Object thread : threadsCopy) { + System.out.println(prefix + (Thread)thread); } - for (Iterator it = groupsListCopy.iterator(); it.hasNext();) { - ((ThreadGroup)it.next()).list(prefix); + for (Object group : groupsCopy) { + ((ThreadGroup)group).list(prefix); } } @@ -597,24 +585,26 @@ * will be thrown. * @return false if this ThreadGroup is not empty */ - private boolean nonsecureDestroy() { - boolean thisGroupIsEmpty = true; + private void nonsecureDestroy() { + + List groupsCopy = null; + synchronized (lock) { - if (!threads.isEmpty()) { - return false; + if (threads.size() > 0) { + throw new IllegalThreadStateException("The thread group " + name + + " is not empty"); } - for (Iterator it = groups.iterator(); it.hasNext();) { - if (it.next().nonsecureDestroy()) { - it.remove(); - } else { - thisGroupIsEmpty = false; - } - } - if (groups.isEmpty()) { - destroyed = true; - } + destroyed = true; + groupsCopy = (List)groups.clone(); } - return thisGroupIsEmpty; + + if (parent != null) { + parent.remove(this); + } + + for (Object group : groupsCopy) { + ((ThreadGroup)group).nonsecureDestroy(); + } } /** @@ -623,12 +613,11 @@ */ private void nonsecureInterrupt() { synchronized (lock) { - Object[] threadsCopy = copyThreads(); // a copy of threads list - for (int i = 0; i < threadsCopy.length; i++) { - ((Thread) threadsCopy[i]).interrupt(); + for (Object thread : threads) { + ((Thread)thread).interrupt(); } - for (Iterator it = groups.iterator(); it.hasNext();) { - ((ThreadGroup)it.next()).nonsecureInterrupt(); + for (Object group : groups) { + ((ThreadGroup)group).nonsecureInterrupt(); } } } @@ -639,12 +628,11 @@ */ private void nonsecureResume() { synchronized (lock) { - Object[] threadsCopy = copyThreads(); - for (int i = 0; i < threadsCopy.length; i++) { - ((Thread) threadsCopy[i]).resume(); + for (Object thread : threads) { + ((Thread)thread).resume(); } - for (Iterator it = groups.iterator(); it.hasNext();) { - ((ThreadGroup)it.next()).nonsecureResume(); + for (Object group : groups) { + ((ThreadGroup)group).nonsecureResume(); } } } @@ -657,8 +645,8 @@ synchronized (lock) { this.maxPriority = priority; - for (Iterator it = groups.iterator(); it.hasNext();) { - ((ThreadGroup)it.next()).nonsecureSetMaxPriority(priority); + for (Object group : groups) { + ((ThreadGroup)group).nonsecureSetMaxPriority(priority); } } } @@ -669,12 +657,11 @@ */ private void nonsecureStop() { synchronized (lock) { - Object[] threadsCopy = copyThreads(); - for (int i = 0; i < threadsCopy.length; i++) { - ((Thread) threadsCopy[i]).stop(); + for (Object thread : threads) { + ((Thread)thread).stop(); } - for (Iterator it = groups.iterator(); it.hasNext();) { - ((ThreadGroup)it.next()).nonsecureStop(); + for (Object group : groups) { + ((ThreadGroup)group).nonsecureStop(); } } } @@ -685,12 +672,11 @@ */ private void nonsecureSuspend() { synchronized (lock) { - Object[] threadsCopy = copyThreads(); // a copy of threads list - for (int i = 0; i < threadsCopy.length; i++) { - ((Thread) threadsCopy[i]).suspend(); + for (Object thread : threads) { + ((Thread)thread).suspend(); } - for (Iterator it = groups.iterator(); it.hasNext();) { - ((ThreadGroup)it.next()).nonsecureSuspend(); + for (Object group : groups) { + ((ThreadGroup)group).nonsecureSuspend(); } } } Index: vm/tests/kernel/java/lang/ThreadGroupTest.java =================================================================== --- vm/tests/kernel/java/lang/ThreadGroupTest.java (revision 581320) +++ vm/tests/kernel/java/lang/ThreadGroupTest.java (working copy) @@ -273,26 +273,33 @@ */ public void testActiveGroupCount_DestroyNonEmptySubgroup() { ThreadGroup tg1 = new ThreadGroup("group 1"); - new ThreadGroup(tg1, "group 11"); + ThreadGroup tg11 = new ThreadGroup(tg1, "group 11"); ThreadGroup tg12 = new ThreadGroup(tg1, "group 12"); - new ThreadGroup(tg1, "group 13"); - new ThreadGroup(tg12, "group 121"); - new ThreadGroup(tg12, "group 122"); + ThreadGroup tg13 = new ThreadGroup(tg1, "group 13"); + ThreadGroup tg121 = new ThreadGroup(tg12, "group 121"); + ThreadGroup tg122 = new ThreadGroup(tg12, "group 122"); ThreadGroup tg123 = new ThreadGroup(tg12, "group 123"); - new ThreadTest.ThreadRunning(tg123, "thread 1231").start(); - new ThreadTest.ThreadRunning(tg123, "thread 1232").start(); - // tg1 and its non-empty subgroups tg123 and tg12 - // should not be destroyed. - // All empty subgroups shoud be destroyed. + new ThreadTest.ThreadRunning(tg122, "thread 1221").start(); + new ThreadTest.ThreadRunning(tg122, "thread 1222").start(); + // Non-empty subgroup tg122 should not be destroyed. // IllegalThreadStateException should be thrown. + // Groups residing on the right from tg123 in the groups tree + // should not be destroyed as well. try { tg1.destroy(); fail("IllegalThreadStateException has not been thrown when " + "destroying non-empty subgroup"); } catch (IllegalThreadStateException e) { } - assertEquals("wrong group count in tg1", 2, tg1.activeGroupCount()); - assertEquals("wrong group count in tg12", 1, tg12.activeGroupCount()); + assertEquals("wrong group count in tg1", 0, tg1.activeGroupCount()); + assertEquals("wrong group count in tg12", 0, tg12.activeGroupCount()); + assertTrue("tg1 is not destroyed", tg1.isDestroyed()); + assertTrue("tg11 is not destroyed", tg11.isDestroyed()); + assertTrue("tg12 is not destroyed", tg12.isDestroyed()); + assertTrue("tg13 is destroyed", !tg13.isDestroyed()); + assertTrue("tg121 is not destroyed", tg121.isDestroyed()); + assertTrue("tg122 is destroyed", !tg122.isDestroyed()); + assertTrue("tg123 is destroyed", !tg123.isDestroyed()); } /**