Merge pull request #31433 from mswatosh/DataLock

Fix for Data deadlock with EJB in .war

Author: mswatosh

dev/io.openliberty.data.internal.persistence/src/io/openliberty/data/internal/persistence/DataProvider.java

@@ -212,14 +212,13 @@ public class DataProvider implements //
                     new ConcurrentHashMap<>();
 
     /**
-     * EntityManagerBuilder futures, grouped by application, for EJB modules
-     * that are triggered to initialize on module starting rather than waiting
-     * for application start.
+     * EntityManagerBuilder futures, grouped by application, triggered to initialize
+     * on module starting rather than waiting for application start.
      * The Set values in this map are subsets of the Set values in futureEMBuilders.
      * The entries are removed on application start, which uses the values to avoid
      * duplicated initalization attempts.
      */
-    private final ConcurrentHashMap<String, Set<FutureEMBuilder>> futureEMBuildersInEJB = //
+    private final ConcurrentHashMap<String, Set<FutureEMBuilder>> processedFutureEMBuilders = //
                     new ConcurrentHashMap<>();
 
     /**
@@ -326,7 +325,7 @@ public void applicationStarted(ApplicationInfo appInfo) throws StateChangeExcept
         //Use deployment name but fall back to generated name
         String appName = appInfo.getDeploymentName() == null ? appInfo.getName() : appInfo.getDeploymentName();
         Set<FutureEMBuilder> futures = futureEMBuilders.get(appName);
-        Set<FutureEMBuilder> skip = futureEMBuildersInEJB.remove(appName);
+        Set<FutureEMBuilder> skip = processedFutureEMBuilders.remove(appName);
         if (futures != null) {
             for (FutureEMBuilder futureEMBuilder : futures) {
                 if (skip == null || !skip.contains(futureEMBuilder))
@@ -410,6 +409,7 @@ public void componentMetaDataCreated(MetaDataEvent<ComponentMetaData> event) {
 
         if (TraceComponent.isAnyTracingEnabled() && tc.isDebugEnabled())
             Tr.debug(this, tc, "componentMetaDataCreated " + jeeName, metadata);
+
     }
 
     @Override
@@ -880,50 +880,39 @@ public void moduleStarted(ModuleInfo moduleInfo) throws StateChangeException {
     public void moduleStarting(ModuleInfo moduleInfo) throws StateChangeException {
         final boolean trace = TraceComponent.isAnyTracingEnabled();
 
-        // TODO here and elsewhere: use whichever of getDeploymentName() or getName()
-        // is guaranteed to match J2EEName
-        String appName = moduleInfo.getApplicationInfo().getName();
+        ApplicationInfo appInfo = moduleInfo.getApplicationInfo();
+        //Use deployment name but fall back to generated name
+        String appName = appInfo.getDeploymentName() == null ? appInfo.getName() : appInfo.getDeploymentName();
         String moduleName = moduleInfo.getName(); // does not include .jar at the end
 
         if (trace && tc.isDebugEnabled())
             Tr.debug(this, tc, "moduleStarting " + moduleInfo, appName, moduleName);
 
-        Set<FutureEMBuilder> processed = futureEMBuildersInEJB.get(appName);
+        Set<FutureEMBuilder> processed = processedFutureEMBuilders.get(appName);
 
         // TODO it would be more direct to map from appName+moduleName -> FutureEMBuilder,
         // but we would need to take into account the difference in module names
         // including or not including .jar at the end.
         Set<FutureEMBuilder> futures = futureEMBuilders.get(appName);
         if (futures != null)
             for (FutureEMBuilder futureEMBuilder : futures) {
-                // The JEE name includes .jar at the end of EJB modules
-                String moduleNameWithDot = futureEMBuilder.jeeName.getModule();
-
-                if (!futureEMBuilder.inWebModule &&
-                    moduleNameWithDot != null &&
-                    moduleNameWithDot.length() == moduleName.length() + 4 &&
-                    moduleNameWithDot.startsWith(moduleName) &&
-                    moduleNameWithDot.endsWith(".jar")) {
-
-                    if (trace && tc.isDebugEnabled())
-                        Tr.debug(this, tc, "matched with " + futureEMBuilder.jeeName);
-
-                    // This delays createEMBuilder until restore.
-                    // While this works by avoiding all connections to the data source, it does make restore much slower.
-                    // TODO figure out how to do more work on restore without having to make a connection to the data source
-                    CheckpointPhase.onRestore(() -> futureEMBuilder.completeAsync(futureEMBuilder::createEMBuilder, executor));
+                // This delays createEMBuilder until restore.
+                // While this works by avoiding all connections to the data source, it does make restore much slower.
+                // TODO figure out how to do more work on restore without having to make a connection to the data source
+                CheckpointPhase.onRestore(() -> futureEMBuilder.completeAsync(futureEMBuilder::createEMBuilder, executor));
+
+                if (processed == null) {
+                    processed = new ConcurrentSkipListSet<>();
+                    Set<FutureEMBuilder> previous = processedFutureEMBuilders //
+                                    .putIfAbsent(appName, processed);
+                    if (previous != null)
+                        processed = previous;
+                }
 
-                    if (processed == null) {
-                        processed = new ConcurrentSkipListSet<>();
-                        Set<FutureEMBuilder> previous = futureEMBuildersInEJB //
-                                        .putIfAbsent(appName, processed);
-                        if (previous != null)
-                            processed = previous;
-                    }
+                processed.add(futureEMBuilder);
 
-                    processed.add(futureEMBuilder);
-                }
             }
+
     }
 
     @Override

dev/io.openliberty.data.internal.persistence/src/io/openliberty/data/internal/persistence/cdi/FutureEMBuilder.java

@@ -296,7 +296,7 @@ public EntityManagerBuilder createEMBuilder() {
         final boolean trace = TraceComponent.isAnyTracingEnabled();
         try {
             String resourceName = dataStore;
-            String metadataIdentifier = getMetadataIdentifier();
+            String metadataIdentifier = getMetadataIdentifier(inWebModule);
 
             // metadataIdentifier examples:
             // WEB#MyApp#MyWebModule.war
@@ -335,8 +335,13 @@ public EntityManagerBuilder createEMBuilder() {
                         metadata = (ComponentMetaData) provider.metadataIdSvc //
                                         .getMetaData(metadataIdentifier);
                     } catch (IllegalStateException x) {
-                        if (System.nanoTime() - start > TimeUnit.SECONDS.toNanos(30))
-                            throw x;
+                        try { //Check if this is an EJB in a Web Module
+                            metadata = (ComponentMetaData) provider.metadataIdSvc //
+                                            .getMetaData(getMetadataIdentifier(false));
+                        } catch (IllegalStateException y) {
+                            if (System.nanoTime() - start > TimeUnit.SECONDS.toNanos(30))
+                                throw x; //Throw exception for non-EJB check
+                        }
                         // else retry because the deferred metadata is not available yet
                     }
                 } while (metadata == null);
@@ -672,11 +677,14 @@ private J2EEName getArtifactName(Class<?> repositoryInterface,
      * Obtains the metadata identifier for the module that defines the repository
      * interface.
      *
+     * @param isWebComponent true for a Web Module, false for an EJB Module
+     *
      * @return metadata identifier as the key, and application/module/component
      *         as the value. Module and component might be null or might not be
      *         present at all.
      */
-    private String getMetadataIdentifier() {
+    private String getMetadataIdentifier(boolean isWebComponent) {
+
         String mdIdentifier;
 
         if (jeeName.getModule() == null) {
@@ -685,7 +693,7 @@ private String getMetadataIdentifier() {
                                                           null);
         } else {
             mdIdentifier = provider.metadataIdSvc //
-                            .getMetaDataIdentifier(inWebModule ? "WEB" : "EJB",
+                            .getMetaDataIdentifier(isWebComponent ? "WEB" : "EJB",
                                                    jeeName.getApplication(),
                                                    jeeName.getModule(),
                                                    null);

dev/io.openliberty.data.internal_fat/test-applications/DataTestApp/src/test/jakarta/data/web/DataTestEJB.java

@@ -15,6 +15,7 @@
 import jakarta.annotation.PostConstruct;
 import jakarta.ejb.Singleton;
 import jakarta.ejb.Startup;
+import jakarta.inject.Inject;
 
 /**
  * A singleton startup EJB that is included inside the war file.
@@ -24,11 +25,11 @@
 public class DataTestEJB {
 
     //TODO renable after fixing deadlock in DataProvider
-//    @Inject
-//    Animals animals;
+    @Inject
+    Animals animals;
 
     @PostConstruct
     public void init() {
-//        animals.findAll();
+        animals.findAll();
     }
 }