pjfanning commented on code in PR #158: URL: https://github.com/apache/poi-xmlbeans/pull/158#discussion_r4456199330
########## src/test/java/misc/detailed/StaticInitializationRingTest.java: ########## @@ -0,0 +1,87 @@ +/* Copyright 2004 The Apache Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package misc.detailed; + +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * XMLBEANS-616: two threads that touch XMLBeans for the first time at once, one through {@code XmlObject.Factory} and + * one through a generated type, must not wait for each other forever. + * + * <p>{@code XmlBeans.<clinit>} needs {@code SchemaTypeLoaderBase} (for {@code NO_TYPE}, through its subclass + * {@code BuiltinSchemaTypeSystem}); a static field of {@code SchemaTypeLoaderBase} read {@code XmlBeans} in turn. The + * first thread then held the initialization of {@code XmlBeans}, the second that of {@code SchemaTypeLoaderBase}, and + * each waited for the other's. Every attempt loads XMLBeans from the test class path in a class loader of its own, so + * its classes start uninitialized; the generated type is XMLBeans' own {@code SchemaDocument}.</p> + */ +public class StaticInitializationRingTest { + + private static final int ATTEMPTS = 20; + + private static final long TIMEOUT_MILLIS = 5000; + + @Test + void firstContactFromBothEndsDoesNotDeadlock() throws Exception { + URL[] classPath = testClassPath(); + int hung = 0; + for (int attempt = 0; attempt < ATTEMPTS && hung == 0; attempt++) { + URLClassLoader loader = new URLClassLoader(classPath, ClassLoader.getPlatformClassLoader()); Review Comment: The loader is never closed, so a passing run opens 20 `URLClassLoader`s over the whole test class path and keeps their jar handles open in the test JVM. Closing it when neither thread hung (try-with-resources, or `close()` after the joins) would avoid that. If a thread is still stuck it's fine to leave that one open. ########## src/test/java/misc/detailed/StaticInitializationRingTest.java: ########## @@ -0,0 +1,87 @@ +/* Copyright 2004 The Apache Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package misc.detailed; + +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * XMLBEANS-616: two threads that touch XMLBeans for the first time at once, one through {@code XmlObject.Factory} and + * one through a generated type, must not wait for each other forever. + * + * <p>{@code XmlBeans.<clinit>} needs {@code SchemaTypeLoaderBase} (for {@code NO_TYPE}, through its subclass + * {@code BuiltinSchemaTypeSystem}); a static field of {@code SchemaTypeLoaderBase} read {@code XmlBeans} in turn. The + * first thread then held the initialization of {@code XmlBeans}, the second that of {@code SchemaTypeLoaderBase}, and + * each waited for the other's. Every attempt loads XMLBeans from the test class path in a class loader of its own, so + * its classes start uninitialized; the generated type is XMLBeans' own {@code SchemaDocument}.</p> + */ +public class StaticInitializationRingTest { + + private static final int ATTEMPTS = 20; + + private static final long TIMEOUT_MILLIS = 5000; + + @Test + void firstContactFromBothEndsDoesNotDeadlock() throws Exception { + URL[] classPath = testClassPath(); + int hung = 0; + for (int attempt = 0; attempt < ATTEMPTS && hung == 0; attempt++) { + URLClassLoader loader = new URLClassLoader(classPath, ClassLoader.getPlatformClassLoader()); + CountDownLatch start = new CountDownLatch(1); + Thread untyped = touch(loader, start, "org.apache.xmlbeans.XmlObject"); + Thread typed = touch(loader, start, "org.apache.xmlbeans.impl.xb.xsdschema.SchemaDocument"); + start.countDown(); + untyped.join(TIMEOUT_MILLIS); + typed.join(TIMEOUT_MILLIS); + if (untyped.isAlive() || typed.isAlive()) { + hung++; + } + } + assertEquals(0, hung, "the first contact from both ends of the static initialization deadlocked"); + } + + private static URL[] testClassPath() throws MalformedURLException { + List<URL> urls = new ArrayList<>(); + for (String entry : System.getProperty("java.class.path").split(File.pathSeparator)) { + urls.add(new File(entry).toURI().toURL()); + } + return urls.toArray(new URL[0]); + } + + private static Thread touch(ClassLoader loader, CountDownLatch start, String type) { + Thread thread = new Thread(() -> { + try { + start.await(); + Object factory = Class.forName(type, true, loader).getField("Factory").get(null); + factory.getClass().getMethod("newInstance").invoke(factory); + } catch (Exception e) { + throw new IllegalStateException(e); Review Comment: An exception here only ends the worker thread, and the test checks nothing but `isAlive()`. If `Class.forName`, `getField("Factory")` or `newInstance` fails (class path problem, renamed class, etc.), both threads die immediately and the test passes without testing anything. Could you record any `Throwable` from each thread (e.g. an `AtomicReference<Throwable>` or a `FutureTask`) and fail the test if one is set? ########## src/test/java/misc/detailed/StaticInitializationRingTest.java: ########## @@ -0,0 +1,87 @@ +/* Copyright 2004 The Apache Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package misc.detailed; + +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * XMLBEANS-616: two threads that touch XMLBeans for the first time at once, one through {@code XmlObject.Factory} and + * one through a generated type, must not wait for each other forever. + * + * <p>{@code XmlBeans.<clinit>} needs {@code SchemaTypeLoaderBase} (for {@code NO_TYPE}, through its subclass + * {@code BuiltinSchemaTypeSystem}); a static field of {@code SchemaTypeLoaderBase} read {@code XmlBeans} in turn. The + * first thread then held the initialization of {@code XmlBeans}, the second that of {@code SchemaTypeLoaderBase}, and + * each waited for the other's. Every attempt loads XMLBeans from the test class path in a class loader of its own, so + * its classes start uninitialized; the generated type is XMLBeans' own {@code SchemaDocument}.</p> + */ +public class StaticInitializationRingTest { + + private static final int ATTEMPTS = 20; Review Comment: Minor: each attempt loads XMLBeans from scratch, so 20 attempts probably adds several seconds to every build. The probe caught the hang in about half the attempts, so something like 10 would still catch a regression reliably. Not blocking. ########## src/test/java/misc/detailed/StaticInitializationRingTest.java: ########## @@ -0,0 +1,87 @@ +/* Copyright 2004 The Apache Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package misc.detailed; + +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * XMLBEANS-616: two threads that touch XMLBeans for the first time at once, one through {@code XmlObject.Factory} and + * one through a generated type, must not wait for each other forever. + * + * <p>{@code XmlBeans.<clinit>} needs {@code SchemaTypeLoaderBase} (for {@code NO_TYPE}, through its subclass + * {@code BuiltinSchemaTypeSystem}); a static field of {@code SchemaTypeLoaderBase} read {@code XmlBeans} in turn. The + * first thread then held the initialization of {@code XmlBeans}, the second that of {@code SchemaTypeLoaderBase}, and + * each waited for the other's. Every attempt loads XMLBeans from the test class path in a class loader of its own, so + * its classes start uninitialized; the generated type is XMLBeans' own {@code SchemaDocument}.</p> + */ +public class StaticInitializationRingTest { + + private static final int ATTEMPTS = 20; + + private static final long TIMEOUT_MILLIS = 5000; + + @Test + void firstContactFromBothEndsDoesNotDeadlock() throws Exception { + URL[] classPath = testClassPath(); + int hung = 0; + for (int attempt = 0; attempt < ATTEMPTS && hung == 0; attempt++) { + URLClassLoader loader = new URLClassLoader(classPath, ClassLoader.getPlatformClassLoader()); + CountDownLatch start = new CountDownLatch(1); + Thread untyped = touch(loader, start, "org.apache.xmlbeans.XmlObject"); + Thread typed = touch(loader, start, "org.apache.xmlbeans.impl.xb.xsdschema.SchemaDocument"); + start.countDown(); + untyped.join(TIMEOUT_MILLIS); + typed.join(TIMEOUT_MILLIS); + if (untyped.isAlive() || typed.isAlive()) { + hung++; + } + } + assertEquals(0, hung, "the first contact from both ends of the static initialization deadlocked"); + } + + private static URL[] testClassPath() throws MalformedURLException { + List<URL> urls = new ArrayList<>(); + for (String entry : System.getProperty("java.class.path").split(File.pathSeparator)) { Review Comment: Minor: this depends on the tests running on the class path (true for the current Gradle `test` task). If that changed to the module path, the classes wouldn't be found. With the exception handling above fixed, that would show up as a failure instead of a silent pass, so no change needed beyond that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
