minor improvements

diff --git a/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java b/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java
index 5452270503767b834cd0d33ff6cf333cc2d43c64..555e6bb2bdd145023da962bf8a13a260622c49a8 100644 (file)
--- a/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java
+++ b/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java
@@ -189,9 +189,16 @@ public class LocalVariableTable extends Attribute {
                return buf.toString();
        }
 
-       @Override
-       public Object clone() throws CloneNotSupportedException {
-               return super.clone();
+       /**
+        * Returns copy of this attribute using same packed state. Used in unit tests.
+        */
+       public synchronized LocalVariableTable copyFromPackedState() {
+               if (!isInPackedState) throw new IllegalStateException("No in packed state");
+               try {
+                       return new LocalVariableTable(nameIndex, length, new DataInputStream(new ByteArrayInputStream(data)), getConstantPool());
+               } catch (IOException e) {
+                       throw new RuntimeException("Failed to unpack clone", e);
+               }
        }
 
        /**

diff --git a/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java b/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java
index da9b8c296482573875bbc2b3ca3a9406d395dc71..b62ffe3806cb64e72a9aee5e86227d3d76bd6395 100644 (file)
--- a/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java
+++ b/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java
@@ -7,7 +7,7 @@
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- *     Andy Clement -     initial implementation
+ *     Dmitry Mikhaylov - initial implementation
  * ******************************************************************/
 
 package org.aspectj.apache.bcel.classfile.tests;
@@ -16,7 +16,6 @@ import org.aspectj.apache.bcel.classfile.Code;
 import org.aspectj.apache.bcel.classfile.JavaClass;
 import org.aspectj.apache.bcel.classfile.LocalVariableTable;
 import org.aspectj.apache.bcel.classfile.Method;
-import org.aspectj.apache.bcel.classfile.tests.BcelTestCase;
 
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
@@ -35,29 +34,31 @@ public class LocalVariableTableConcurrencyTest extends BcelTestCase {
 
        protected void setUp() throws Exception {
                super.setUp();
+
                for (int i = 0; i < nThreads; i++) workers[i] = Executors.newSingleThreadExecutor();
 
                JavaClass clazz = getClassFromJar("SimpleGenericsProgram");
-
                Method mainMethod = getMethod(clazz,"main");
                Code codeAttr = (Code) findAttribute("Code",mainMethod.getAttributes());
-
-               reference =
-                               (LocalVariableTable) findAttribute("LocalVariableTable",codeAttr.getAttributes());
-       }
-
-       private LocalVariableTable createReferenceCopy() {
-               try {
-                       return (LocalVariableTable) reference.clone();
-               } catch (CloneNotSupportedException e) {
-                       throw new RuntimeException("Faileed to clone LocalVariableTable", e);
-               }
+               reference = (LocalVariableTable) findAttribute("LocalVariableTable",codeAttr.getAttributes());
        }
 
+       /**
+        * Try to hit concurrency bug in org.aspectj.apache.bcel.classfile.LocalVariableTable.unpack().
+        * We do so by running unpack() on same instance with multiple threads, and artificially
+        * delaying all threads but first so that they enter unpack() the moment first thread is about to leave it.
+        *
+        * Since this test relies on empirically obtained access pattern and number of iterations,
+        * it never has 100% probability of hitting the bug. If it fails - there is certainly a bug.
+        * If it passes, it could mean anything - fully correct code or slightly changed execution order preventing
+        * threads to collide at problematic location.
+        *
+        * As such, it is not really good for unit testing.
+        */
        public void testLocalVariableTableAttributeConcurrency() throws RuntimeException, InterruptedException {
                final AtomicReference<RuntimeException> error = new AtomicReference<>();
-               for (int i = 0; i < 100000; i++) {
-                       LocalVariableTable sharedInstance = createReferenceCopy();
+               for (int i = 0; i < 10000; i++) {
+                       LocalVariableTable sharedInstance = reference.copyFromPackedState();
                        CountDownLatch preStart = new CountDownLatch(nThreads);
                        Semaphore start = new Semaphore(0);
                        CountDownLatch finish = new CountDownLatch(nThreads);
@@ -67,8 +68,8 @@ public class LocalVariableTableConcurrencyTest extends BcelTestCase {
                                workers[j].execute(() -> {
                                        preStart.countDown();
                                        start.acquireUninterruptibly();
-                                       // trying to trigger concurrent unpacking - one tread should enter unpack() when other is about to leave it
-                                       if (needsDelay) createReferenceCopy().getTableLength();
+                                       // trying to trigger concurrent unpacking bug - one tread should enter unpack() when other is about to leave it
+                                       if (needsDelay) reference.copyFromPackedState().getTableLength();
                                        try {
                                                sharedInstance.getTableLength();
                                        }