changeset 8319:20162088f4f8

Improved (failing) ConcurrentTypeTest Part of the investigation of #2834, adds use of Thread.sleep to some of the test scripts, access to which fails quite frequently. The case testSeparateLoader2() does not produce failures because incorrect behaviour is currently not tested.
author Jeff Allen <ja.py@farowl.co.uk>
date Thu, 23 Jan 2020 15:12:48 +0000
parents e306270a4771
children 44280f7e1854
files src/org/python/core/packagecache/SysPackageManager.java tests/java/org/python/core/ConcurrentTypeTest.java
diffstat 2 files changed, 122 insertions(+), 50 deletions(-) [+]
line wrap: on
line diff
--- a/src/org/python/core/packagecache/SysPackageManager.java
+++ b/src/org/python/core/packagecache/SysPackageManager.java
@@ -240,9 +240,9 @@ public class SysPackageManager extends P
 
         doDir(this.searchPath, ret, jpkg, instantiate, exclpkgs);
 
-        PySystemState system = Py.getSystemState();
-        if (system.getClassLoader() == null) {
-            doDir(system.path, ret, jpkg, instantiate, exclpkgs);
+        PySystemState sys = Py.getSystemState();
+        if (sys.getClassLoader() == null) {
+            doDir(sys.path, ret, jpkg, instantiate, exclpkgs);
         }
 
         return merge(basic, ret);
--- a/tests/java/org/python/core/ConcurrentTypeTest.java
+++ b/tests/java/org/python/core/ConcurrentTypeTest.java
@@ -5,8 +5,8 @@ package org.python.core;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertNotNull;
 
-import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -43,7 +43,7 @@ import org.python.util.PythonInterpreter
  */
 public class ConcurrentTypeTest {
 
-    private static int RUNNERS = 30;
+    private static int RUNNERS = 100;
 
     static {
         // Do not need site.py for test: makes more complicated in IDE.
@@ -52,20 +52,30 @@ public class ConcurrentTypeTest {
 
     private abstract static class ScriptRunner implements Runnable {
 
-        final String script;
+        final int instance;
         final Thread thread;
+        String script;
+        PyCode code;
         final PyStringMap globals = Py.newStringMap();
         /** Sub-class constructor must assign the configured interpreter. */
-        protected PythonInterpreter interp;
+        PythonInterpreter interp;
 
-        ScriptRunner(String script) {
+        ScriptRunner(int instance) {
+            this.instance = instance;
+            this.thread = new Thread(this);
+            this.globals.__setitem__("instance", Py.newInteger(instance));
+        }
+
+        /** Set the interpreter and compile the script. */
+        void setInterp(PythonInterpreter interp, String script) {
+            this.interp = interp;
             this.script = script;
-            this.thread = new Thread(this);
+            this.code = interp.compile(script);
         }
 
         @Override
         public void run() {
-            interp.exec(script);
+            interp.exec(code);
         }
     }
 
@@ -79,7 +89,7 @@ public class ConcurrentTypeTest {
         // Make all the runners in advance.
         List<SharedStateRunner> runners = new ArrayList<>(RUNNERS);
         for (int i = 0; i < RUNNERS; i++) {
-            runners.add(new SharedStateRunner(javaImportScript));
+            runners.add(new SharedStateRunner(javaImportScript, i));
         }
 
         // Start the runners then let all of them finish (or fail).
@@ -87,9 +97,8 @@ public class ConcurrentTypeTest {
 
         // Check status of every thread
         for (SharedStateRunner r : runners) {
-            PyObject status = r.globals.__finditem__("status");
-            assertTrue("status not set to an int", status instanceof PyInteger);
-            assertEquals(((PyInteger) status).asInt(), 1);
+            PyObject e = r.globals.__finditem__("e");
+            assertEquals("Runner " + r.instance, null, e);
         }
     }
 
@@ -99,30 +108,39 @@ public class ConcurrentTypeTest {
      */
     //@formatter:off
     static final String javaImportScript = String.join("\n", new String[] {
-        "from javax.swing.text.Utilities import *",
-        "try:",
-        "    f = getNextWord",
-        "    status = 1",
-        "except Exception:",
-        "    status = 0"
+       "try:",
+       "    from javax.swing.text.Utilities import *",
+       "    f = getNextWord", // May raise NameError
+       "except Exception as e:",
+       "    pass"
     });
     //@formatter:on
 
     /**
-     * Each instance of this type has its own interpreter, but they all share the same (default)
-     * {@code PySystemState}
+     * Each instance of this type has its own {@link PythonInterpreter}, but they all share the same
+     * (default) {@code PySystemState}
      */
     private static class SharedStateRunner extends ScriptRunner {
 
-        SharedStateRunner(String script) {
-            super(script);
-            this.interp = new PythonInterpreter(globals);
+        SharedStateRunner(String script, int instance) {
+            super(instance);
+            setInterp(new PythonInterpreter(globals), script);
         }
     }
 
     /**
      * Test concurrency when importing the same Java class where the interpreters all have their own
-     * {@code PySystemState}.
+     * {@code PySystemState}. At version 2.7.2b2, this test occasionally fails with a
+     * {@code NameError}, when the member {@code getNextWord} imported from
+     * {@code javax.swing.text.Utilities} is not found. The cause is thought to be a race between
+     * threads in {@code SysPackageManager}. It is necessary to run this test in the shell with
+     * something like:<pre>
+     * PS jython-trunk> do {
+     * >>   java -cp "build\exposed;build\classes;extlibs\*" org.junit.runner.JUnitCore org.python.core.ConcurrentTypeTest
+     * >> } while ($LastExitCode -eq 0)
+     * </pre> Expect a hundred iterations or more before you see
+     * {@code "name 'getNextWord' is not defined"}. Suppress other tests in the suite.
+     *
      */
     @Test
     public void testSeparateState() {
@@ -130,7 +148,7 @@ public class ConcurrentTypeTest {
         // Make all the runners in advance.
         List<SeparateStateRunner> runners = new ArrayList<>(RUNNERS);
         for (int i = 0; i < RUNNERS; i++) {
-            runners.add(new SeparateStateRunner(javaImportScript));
+            runners.add(new SeparateStateRunner(javaImportScript, i));
         }
 
         // Start the runners then let all of them finish (or fail).
@@ -138,24 +156,67 @@ public class ConcurrentTypeTest {
 
         // Check status of every thread
         for (SeparateStateRunner r : runners) {
-            PyObject status = r.globals.__finditem__("status");
-            assertTrue("status not set to an int", status instanceof PyInteger);
-            assertEquals(((PyInteger) status).asInt(), 1);
+            PyObject e = r.globals.__finditem__("e");
+            assertEquals("Runner " + r.instance, null, e);
         }
     }
 
     /**
-     * Each instance of this type has its own {@code PySystemState}, as well as its own interpreter.
+     * Script to import all names from a Java class for {@link #testSharedState()} and
+     * {@link #testSeparateState()}. At version 2.7.2b2, this test frequently (not every time) fails
+     * with an {@code AttributeError}, when member {@code sleep} of {@code java.lang.Thread} is not
+     * found for the call.
+     */
+    //@formatter:off
+    static final String javaImportScript2 = String.join("\n", new String[] {
+       "try:",
+       "    from java.lang import Thread",
+       "    Thread.sleep(instance*10)",  // May raise AttributeError
+       "    from javax.swing.text.Utilities import *",
+       "    f = getNextWord", // May raise NameError
+       "except Exception as e:",
+       "    pass"
+    });
+    //@formatter:on
+
+    /**
+     * Test concurrency when importing the same Java class where the interpreters all have their own
+     * {@code PySystemState}. It differs from {@link #testSeparateState()} in importing
+     * {@code Thread} add calling {@code Thread.sleep}. (The original idea was to provoke
+     * switching.) At version 2.7.2b2, this test fails about one time in 5 with an AttributeError,
+     * when the member {@code Thread.sleep} is not found.
+     */
+    @Test
+    public void testSeparateState2() {
+
+        // Make all the runners in advance.
+        List<SeparateStateRunner> runners = new ArrayList<>(RUNNERS);
+        for (int i = 0; i < RUNNERS; i++) {
+            runners.add(new SeparateStateRunner(javaImportScript2, i));
+        }
+
+        // Start the runners then let all of them finish (or fail).
+        awaitAll(runners);
+
+        // Check status of every thread
+        for (SeparateStateRunner r : runners) {
+            PyObject e = r.globals.__finditem__("e");
+            assertEquals("Runner " + r.instance, null, e);
+        }
+    }
+
+    /**
+     * Each instance of this type has its own {@code PySystemState}, as well as its own
+     * {@link PythonInterpreter}.
      */
     private static class SeparateStateRunner extends ScriptRunner {
 
         final PySystemState sys = new PySystemState();
 
-        SeparateStateRunner(String script) {
-            super(script);
-            this.interp = new PythonInterpreter(globals, sys);
+        SeparateStateRunner(String script, int instance) {
+            super(instance);
+            setInterp(new PythonInterpreter(globals, sys), script);
         }
-
     }
 
     /**
@@ -171,15 +232,18 @@ public class ConcurrentTypeTest {
         // Make all the runners in advance, primed with the same script.
         List<SeparateLoaderRunner> runners = new ArrayList<>(RUNNERS);
         for (int i = 0; i < RUNNERS; i++) {
-            runners.add(new SeparateLoaderRunner(loaderScript, fileManager.newClassLoader()));
+            runners.add(new SeparateLoaderRunner(loaderScript, i, fileManager.newClassLoader()));
         }
 
         // Start the runners then let all of them finish (or fail).
         awaitAll(runners);
 
-        // Check status of every thread
+        // Check result of every thread
         for (SeparateLoaderRunner r : runners) {
+            PyObject e = r.globals.__finditem__("e");
+            assertEquals("Runner " + r.instance, null, e);
             PyObject staticConstant = r.globals.__finditem__("staticConstant");
+            assertNotNull(staticConstant);
             assertEquals(staticConstant.asInt(), 42);
             PyObject x = r.globals.__finditem__("x");
             assertEquals(x.asInt(), 42);
@@ -204,13 +268,16 @@ public class ConcurrentTypeTest {
     //@formatter:on
 
     /**
-     * Script to import all names from a Java class conjured from thin air (via class loader), usaed
+     * Script to import all names from a Java class conjured from thin air (via class loader), used
      * by {@link #testSeparateLoader()}.
      */
     //@formatter:off
     static final String loaderScript = String.join("\n", new String[] {
-        "from thin.air.Foo import *",
-        "x = staticMethod()"
+       "try:",
+       "    from thin.air.Foo import *",
+       "    x = staticMethod()",
+       "except Exception as e:",
+       "    pass"
     });
     //@formatter:on
 
@@ -228,17 +295,19 @@ public class ConcurrentTypeTest {
         // Make all the runners in advance, primed with the same script.
         List<SeparateLoaderRunner> runners = new ArrayList<>(RUNNERS);
         for (int i = 0; i < RUNNERS; i++) {
-            runners.add(new SeparateLoaderRunner(loaderScript2, fileManager.newClassLoader()));
+            runners.add(new SeparateLoaderRunner(loaderScript2, i, fileManager.newClassLoader()));
         }
 
         // Start the runners then let all of them finish (or fail).
         awaitAll(runners);
 
-        // Check status of every thread
+        // Check result of every thread
         Set<Object> classes = new HashSet<>();
         Set<PyType> types = new HashSet<>();
 
         for (SeparateLoaderRunner r : runners) {
+            PyObject e = r.globals.__finditem__("e");
+            assertEquals("Runner " + r.instance, null, e);
             PyObject m = r.globals.__finditem__("m");
             assertEquals(m.toString(), "forty-two");
             PyObject x = r.globals.__finditem__("x");
@@ -260,25 +329,28 @@ public class ConcurrentTypeTest {
      */
     //@formatter:off
     static final String loaderScript2 = String.join("\n", new String[] {
-        "from thin.air import Foo",
-        "f = Foo()",
-        "m = f.member",
-        "x = f.method()"
+       "try:",
+       "    from thin.air import Foo",
+       "    f = Foo()",
+       "    m = f.member",
+       "    x = f.method()",
+       "except Exception as e:",
+       "    pass"
     });
     //@formatter:on
 
     /**
      * Each instance of this type has its own {@code ClassLoader}, as well as its own {@code sys}
-     * module and interpreter.
+     * module and {@link PythonInterpreter}.
      */
     private static class SeparateLoaderRunner extends ScriptRunner {
 
         final PySystemState sys = new PySystemState();
 
-        SeparateLoaderRunner(String script, ClassLoader classLoader) {
-            super(script);
+        SeparateLoaderRunner(String script, int instance, ClassLoader classLoader) {
+            super(instance);
             sys.setClassLoader(classLoader);
-            this.interp = new PythonInterpreter(globals, sys);
+            setInterp(new PythonInterpreter(globals, sys), script);
         }
     }