Skip to content

Commit

Permalink
apacheGH-2787: Deadlock in JenaSystem.init()
Browse files Browse the repository at this point in the history
- Restored the early return from `JenaSystem.init()` as implemented prior to [commit 8654c07](apache@8654c07).
- Simplified initialization using the "Initialization-on-demand holder idiom" for better efficiency and thread safety.
- Added integration tests for `JenaSystem.init()`, including tests for parallel execution cases.
- Implemented additional integration tests that start with a fresh JVM for each run, using JMH as the test runner.
  - Added `org.openjdk.jmh` as a dependency in `jena-integration-tests`.
  • Loading branch information
arne-bdt committed Oct 25, 2024
1 parent 4247422 commit bae591d
Show file tree
Hide file tree
Showing 5 changed files with 262 additions and 8 deletions.
28 changes: 20 additions & 8 deletions jena-core/src/main/java/org/apache/jena/sys/JenaSystem.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,22 +78,34 @@ public static void logLifecycle(String fmt, Object ...args) {

public static void init() {
// Once jena is initialized, all calls are an immediate return.
if ( initialized )
return ;
// Overlapping attempts to perform initialization will block on the synchronized.
synchronized(JenaSystem.class) {
if ( initialized )
return ;
if(initialized) {
return;
}

// Access the initialized flag to trigger class loading
initialized = LazyHolder.IS_INITIALIZED;
}

/**
* Initialization-on-demand holder idiom
* @see <a href="http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom">Initialization-on-demand holder idiom</a>
*
*/
private static class LazyHolder {
public static final boolean IS_INITIALIZED = initialize();

private static boolean initialize() {
initialized = true; // Set early to avoid blocking on static initialization.
setup();
if ( DEBUG_INIT )
singleton.debug(DEBUG_INIT);
singleton.initialize();
singleton.debug(false);
// Last so overlapping initialization waits on the synchronized
initialized = true;
return true;
}
}


public static void shutdown() { singleton.shutdown(); }

/** The level 0 subsystem - inserted without using the Registry load function.
Expand Down
17 changes: 17 additions & 0 deletions jena-integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,18 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-core</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-generator-annprocess</artifactId>
<scope>test</scope>
</dependency>

</dependencies>

<build>
Expand Down Expand Up @@ -186,6 +198,11 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerArgs>
<arg>-proc:full</arg>
</compilerArgs>
</configuration>
</plugin>

<plugin>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you 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 org.apache.jena.sys;

import org.junit.runner.RunWith;
import org.junit.runners.Suite;

@RunWith(Suite.class)
@Suite.SuiteClasses(
{ TestJenaSystem.class
, TestJenaSystemWithFreshJVM.class
})

public class TS_Sys { }
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you 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 org.apache.jena.sys;

import org.apache.jena.rdf.model.ModelFactory;
import org.junit.Test;


import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.HashSet;
import java.util.concurrent.Executors;
import java.util.stream.IntStream;

import static org.junit.jupiter.api.Assertions.*;

public class TestJenaSystem {

/**
* Test that the first initialization of JenaSystem is successful.
* This code was able to reproduce a deadlock for <a href="https://github.com/apache/jena/issues/2787>GitHub issue 2787</a>,
* which is now fixed.
* It needs to be run in a separate JVM to run with a not initialized JenaSystem.
* <p />
* The name is chosen so that it is executed first.
*/
@Test
public void testFirstInitParallel() {

var pool = Executors.newFixedThreadPool(2);
try {
var futures = IntStream.range(1, 3)
.mapToObj(i -> pool.submit(() -> {
if (i % 2 == 0) {
ModelFactory.createDefaultModel();
} else {
JenaSystem.init();
}

return i;
}))
.toList();

var intSet = new HashSet<Integer>();
assertTimeoutPreemptively(
Duration.of(4, ChronoUnit.SECONDS),
() -> {
for (var future : futures) {
intSet.add(future.get());
}
});

assertEquals(2, intSet.size());
} finally {
pool.shutdown();
}
}

/**
* Test that the first initialization of JenaSystem is successful.
* This test is placed in the integration tests module to ensure that the initialization
* is successful when multiple modules are loaded.
*/
@Test
public void testLaterInit() {
assertDoesNotThrow(() -> JenaSystem.init());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you 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 org.apache.jena.sys;

import org.apache.jena.rdf.model.ModelFactory;
import org.junit.Assert;
import org.junit.Test;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.options.OptionsBuilder;
import org.openjdk.jmh.runner.options.TimeValue;

import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.HashSet;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.stream.IntStream;

import static org.junit.jupiter.api.Assertions.*;

/**
* This test is not a benchmark but a test to ensure that the JenaSystem can be initialized in a fresh JVM.
* In <a href="https://github.com/apache/jena/issues/2787>GitHub issue 2787</a>, a deadlock was observed when JenaSystem
* was initialized in parallel with ModelFactory.createDefaultModel() or other classes that use a static initialize to
* initialize JenaSystem. This test is used to reproduce the deadlock and ensure that it is fixed.
*/
@State(Scope.Benchmark)
public class TestJenaSystemWithFreshJVM {

@Benchmark
public void initParallel() {

var pool = Executors.newFixedThreadPool(2);
try {
var futures = IntStream.range(1, 3)
.mapToObj(i -> pool.submit(() -> {
if (i % 2 == 0) {
ModelFactory.createDefaultModel();
}
else {
JenaSystem.init();
}

return i;
}))
.toList();

var intSet = new HashSet<Integer>();
assertTimeoutPreemptively(
Duration.of(4, ChronoUnit.SECONDS),
() -> {
for (var future : futures) {
intSet.add(future.get());
}
});

assertEquals(2, intSet.size());
} finally {
pool.shutdown();
}
}

/**
* Test that the first initialization of JenaSystem is successful.
* This test is placed in the integration tests module to ensure that the initialization
* is successful when multiple modules are loaded.
* <p>
* The test is called as benchmark to ensure that the JVM is fresh.
*/
@Benchmark
public void init() {
JenaSystem.init();
}

@Test
public void benchmark() throws Exception {
var opt = new OptionsBuilder()
.include(this.getClass().getName())
.mode(Mode.AverageTime)
.timeUnit(TimeUnit.SECONDS)
.warmupTime(TimeValue.NONE)
.warmupIterations(0) // we don't need warmup
.measurementIterations(1) // we only need one iteration
.measurementTime(TimeValue.NONE)
.threads(1) // we only need one thread
.forks(1) // we only need one fork
.shouldFailOnError(true) // this is important to fail the test if the benchmark fails
.timeout(TimeValue.seconds(6)) // 6 seconds should be enough, even on slow machines
.build();
var results = new Runner(opt).run();
Assert.assertNotNull(results);
}
}

0 comments on commit bae591d

Please sign in to comment.