diff --git a/odfdom/pom.xml b/odfdom/pom.xml index 6319dab55..753dbdb0f 100644 --- a/odfdom/pom.xml +++ b/odfdom/pom.xml @@ -195,7 +195,7 @@ maven-surefire-plugin false - -javaagent:${settings.localRepository}/org/jacoco/org.jacoco.agent/${jacoco.version}/org.jacoco.agent-${jacoco.version}-runtime.jar=destfile=${project.build.directory}/coverage-reports/jacoco-ut.exec + -javaagent:${settings.localRepository}/org/jacoco/org.jacoco.agent/${jacoco.version}/org.jacoco.agent-${jacoco.version}-runtime.jar=destfile=${project.build.directory}/coverage-reports/jacoco-ut.exec,excludes=${jacoco.agent.excludes} true diff --git a/odfdom/src/main/java/org/odftoolkit/odfdom/dom/style/props/OdfStyleProperty.java b/odfdom/src/main/java/org/odftoolkit/odfdom/dom/style/props/OdfStyleProperty.java index c66ff12a6..f3e8cf9f9 100644 --- a/odfdom/src/main/java/org/odftoolkit/odfdom/dom/style/props/OdfStyleProperty.java +++ b/odfdom/src/main/java/org/odftoolkit/odfdom/dom/style/props/OdfStyleProperty.java @@ -23,8 +23,8 @@ */ package org.odftoolkit.odfdom.dom.style.props; -import java.util.Iterator; -import java.util.TreeSet; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import org.odftoolkit.odfdom.pkg.OdfName; /** @@ -33,15 +33,18 @@ */ public class OdfStyleProperty implements Comparable { - private OdfStylePropertiesSet m_propSet; - private OdfName m_name; + private final OdfStylePropertiesSet m_propSet; + private final OdfName m_name; private OdfStyleProperty(OdfStylePropertiesSet propSet, OdfName name) { m_propSet = propSet; m_name = name; } - private static TreeSet m_styleProperties = new TreeSet<>(); + // Concurrent map, as the static initializers of the generated Style*PropertiesElement classes + // might register their properties from different threads at the same time (see issue #442) + private static final Map m_styleProperties = + new ConcurrentHashMap<>(); /** * Looks if an OdfStyleProperty is already listed in the static sytleProperties set, otherwise @@ -53,26 +56,8 @@ private OdfStyleProperty(OdfStylePropertiesSet propSet, OdfName name) { */ public static OdfStyleProperty get(OdfStylePropertiesSet propSet, OdfName name) { OdfStyleProperty temp = new OdfStyleProperty(propSet, name); - // Replacement for (JDK1.6) - // OdfStyleProperty result = m_styleProperties.floor(temp); - - Iterator iter = m_styleProperties.iterator(); - OdfStyleProperty result = null; - - // check if key exists - if (!m_styleProperties.contains(temp)) { - m_styleProperties.add(temp); - return temp; - } - while (iter.hasNext()) { - result = iter.next(); - if (result.equals(temp)) { - return result; - } - } - - m_styleProperties.add(temp); - return temp; + OdfStyleProperty existing = m_styleProperties.putIfAbsent(temp, temp); + return existing != null ? existing : temp; } /** @return an OdfStylePropertiesSet member */ diff --git a/odfdom/src/test/java/org/odftoolkit/odfdom/dom/style/props/OdfStylePropertyConcurrencyTest.java b/odfdom/src/test/java/org/odftoolkit/odfdom/dom/style/props/OdfStylePropertyConcurrencyTest.java new file mode 100644 index 000000000..d5ab62c36 --- /dev/null +++ b/odfdom/src/test/java/org/odftoolkit/odfdom/dom/style/props/OdfStylePropertyConcurrencyTest.java @@ -0,0 +1,179 @@ +/** + * ********************************************************************** + * + *

DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER + * + *

Copyright 2026 The Document 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. You can also obtain a copy of the License at + * http://odftoolkit.org/docs/license.txt + * + *

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 org.odftoolkit.odfdom.dom.style.props; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.junit.Test; +import org.odftoolkit.odfdom.dom.OdfDocumentNamespace; +import org.odftoolkit.odfdom.pkg.OdfName; + +/** + * Regression tests for concurrent registration of style properties, see + * https://github.com/tdf/odftoolkit/issues/442 + */ +public class OdfStylePropertyConcurrencyTest { + + private static final int THREADS = 32; + private static final int OPERATIONS_PER_THREAD = 5_000; + private static final long TIMEOUT_SECONDS = 60; + + private static final String[] STYLE_PROPERTIES_ELEMENTS = { + "StyleChartPropertiesElement", + "StyleDrawingPagePropertiesElement", + "StyleGraphicPropertiesElement", + "StyleHeaderFooterPropertiesElement", + "StyleListLevelPropertiesElement", + "StylePageLayoutPropertiesElement", + "StyleParagraphPropertiesElement", + "StyleRubyPropertiesElement", + "StyleSectionPropertiesElement", + "StyleTableCellPropertiesElement", + "StyleTableColumnPropertiesElement", + "StyleTablePropertiesElement", + "StyleTableRowPropertiesElement", + "StyleTextPropertiesElement" + }; + + /** Many threads register different new properties at the same time. */ + @Test + public void testConcurrentRegistration() throws Exception { + OdfStylePropertiesSet[] propSets = OdfStylePropertiesSet.values(); + List> results = + runConcurrently( + worker -> { + List props = new ArrayList<>(OPERATIONS_PER_THREAD); + for (int i = 0; i < OPERATIONS_PER_THREAD; i++) { + OdfStylePropertiesSet propSet = propSets[(worker + i) % propSets.length]; + OdfName name = + OdfName.newName( + OdfDocumentNamespace.TEXT, "race-unique-property-" + worker + "-" + i); + OdfStyleProperty prop = OdfStyleProperty.get(propSet, name); + assertSame(propSet, prop.getPropertySet()); + assertSame(name, prop.getName()); + props.add(prop); + } + return props; + }); + // all properties have been registered and are found again + for (List props : results) { + for (OdfStyleProperty prop : props) { + assertSame(prop, OdfStyleProperty.get(prop.getPropertySet(), prop.getName())); + } + } + } + + /** Many threads register the same new properties at the same time and share one instance. */ + @Test + public void testConcurrentCanonicalization() throws Exception { + List> results = + runConcurrently( + worker -> { + List props = new ArrayList<>(OPERATIONS_PER_THREAD); + for (int i = 0; i < OPERATIONS_PER_THREAD; i++) { + props.add( + OdfStyleProperty.get( + OdfStylePropertiesSet.TextProperties, + OdfName.newName(OdfDocumentNamespace.TEXT, "race-shared-property-" + i))); + } + return props; + }); + List expected = results.get(0); + for (List props : results) { + assertEquals(OPERATIONS_PER_THREAD, props.size()); + for (int i = 0; i < OPERATIONS_PER_THREAD; i++) { + assertSame(expected.get(i), props.get(i)); + } + } + } + + /** + * Initializes the generated Style*PropertiesElement classes concurrently, as their static + * initializers register their properties. This is only a real cold start, if no earlier test of + * the same JVM has already loaded these classes. + */ + @Test + public void testConcurrentStylePropertiesElementInitialization() throws Exception { + ClassLoader loader = getClass().getClassLoader(); + List>> results = + runConcurrently( + worker -> { + List> classes = new ArrayList<>(STYLE_PROPERTIES_ELEMENTS.length); + for (int i = 0; i < STYLE_PROPERTIES_ELEMENTS.length; i++) { + String className = + "org.odftoolkit.odfdom.dom.element.style." + + STYLE_PROPERTIES_ELEMENTS[(worker + i) % STYLE_PROPERTIES_ELEMENTS.length]; + classes.add(Class.forName(className, true, loader)); + } + return classes; + }); + for (List> classes : results) { + assertEquals(STYLE_PROPERTIES_ELEMENTS.length, classes.size()); + } + } + + private interface Worker { + T run(int worker) throws Exception; + } + + /** Runs the given work in THREADS threads, all started at the same moment. */ + private static List runConcurrently(Worker work) throws Exception { + ExecutorService pool = Executors.newFixedThreadPool(THREADS); + try { + CountDownLatch ready = new CountDownLatch(THREADS); + CountDownLatch start = new CountDownLatch(1); + List> futures = new ArrayList<>(THREADS); + for (int thread = 0; thread < THREADS; thread++) { + final int worker = thread; + Callable task = + () -> { + ready.countDown(); + start.await(); + return work.run(worker); + }; + futures.add(pool.submit(task)); + } + assertTrue( + "Worker threads did not start in time", ready.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)); + start.countDown(); + + List results = new ArrayList<>(THREADS); + for (Future future : futures) { + // rethrows any failure of a worker thread as ExecutionException + results.add(future.get(TIMEOUT_SECONDS, TimeUnit.SECONDS)); + } + return results; + } finally { + pool.shutdownNow(); + } + } +} diff --git a/pom.xml b/pom.xml index 6c79ba976..e405f2278 100644 --- a/pom.xml +++ b/pom.xml @@ -58,7 +58,10 @@ false yyyy-MM-dd'T'HH:mm:ss - 0.8.14 + 0.8.15 + + java.*:javax.*:jdk.*:sun.*:com.sun.* jacoco reuseReports ${project.reporting.outputDirectory}/jacoco-ut/jacoco.xml @@ -494,6 +497,9 @@ ${project.build.directory}/coverage-reports/jacoco-ut.exec + + ${jacoco.agent.excludes} + ${project.build.directory}/coverage-reports/jacoco-it.exec + + ${jacoco.agent.excludes} +