SDO potential memory leak fix (#1844)

This change solve potential memory leak if SDO component is used in JEE environment.
There is refactor of SDOHelperContext.helperContexts map. Instead of current ConcurrentHashMap is CacheIdentityMap used which supports Least Recently Used (LRU) strategy. This one should be controlled by new SDO system property eclipselink.sdo.helper.contexts.max.size.

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>

sdo/org.eclipse.persistence.sdo/src/main/java/org/eclipse/persistence/sdo/SDOSystemProperties.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2023 Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0,
+ * or the Eclipse Distribution License v. 1.0 which is available at
+ * http://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause
+ */
+
+// Contributors:
+//     Oracle - 4.0.x - initial implementation
+package org.eclipse.persistence.sdo;
+
+/**
+ * This class provides the list of System properties that are recognized by EclipseLink SDO component.
+ * @author rfelcman
+ */
+public final class SDOSystemProperties {
+
+    private SDOSystemProperties() {
+    }
+
+    /**
+     * Property controls strictness of {@link commonj.sdo.Type#getInstanceClass()} type checking.
+     *
+     * <p>
+     * See {@link org.eclipse.persistence.sdo.helper.SDOHelperContext#isStrictTypeCheckingEnabled()} for more details.
+     * By this property, the initial value can be changed.
+     * Default value is {@code true}.
+     * </p>
+     */
+    public static final String SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME = "eclipselink.sdo.strict.type.checking";
+
+    /**
+     * Property controls maximum size of helperContexts map.
+     * This is way how Least Recently Used (LRU) strategy will be intensive. It should help with used Java heap size issues
+     * if there is so many SDOClassLoader instances with so many defined classes.
+     * Default value is {@code 1 000 000}.
+     */
+    public static final String SDO_HELPER_CONTEXTS_MAX_SIZE = "eclipselink.sdo.helper.contexts.max.size";
+}

sdo/org.eclipse.persistence.sdo/src/main/java/org/eclipse/persistence/sdo/helper/SDOHelperContext.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -42,10 +42,11 @@
 
 import org.eclipse.persistence.exceptions.SDOException;
 import org.eclipse.persistence.internal.helper.Helper;
+import org.eclipse.persistence.internal.identitymaps.CacheIdentityMap;
 import org.eclipse.persistence.internal.security.PrivilegedAccessHelper;
 import org.eclipse.persistence.internal.security.PrivilegedGetMethod;
-import org.eclipse.persistence.sdo.SDOConstants;
 import org.eclipse.persistence.sdo.SDOResolvable;
+import org.eclipse.persistence.sdo.SDOSystemProperties;
 import org.eclipse.persistence.sdo.helper.delegates.SDODataFactoryDelegate;
 import org.eclipse.persistence.sdo.helper.delegates.SDOTypeHelperDelegate;
 import org.eclipse.persistence.sdo.helper.delegates.SDOTypeHelperDelegate.SDOWrapperTypeId;
@@ -93,7 +94,7 @@ public class SDOHelperContext implements HelperContext {
     private String identifier;
     private Map<String, Object> properties;
     private boolean isStrictTypeCheckingEnabled = PrivilegedAccessHelper.getSystemPropertyBoolean(
-            STRICT_TYPE_CHECKING_PROPERTY_NAME, true);
+            SDOSystemProperties.SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME, true);
 
     /**
      * Property controls strictness of {@link Type#getInstanceClass()} type checking.
@@ -103,12 +104,19 @@ public class SDOHelperContext implements HelperContext {
      * By this property, the initial value can be changed.
      * Default value is <code>true</code>.
      * </p>
+     *
+     * @deprecated
+     * @see SDOSystemProperties.SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME
+     * Moved to {@link SDOSystemProperties}.
      */
-    public static final String STRICT_TYPE_CHECKING_PROPERTY_NAME = "eclipselink.sdo.strict.type.checking";
+    @Deprecated
+    public static final String STRICT_TYPE_CHECKING_PROPERTY_NAME = SDOSystemProperties.SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME;
+
+    private static int helperContextsMaxSize = Integer.parseInt(PrivilegedAccessHelper.getSystemProperty(SDOSystemProperties.SDO_HELPER_CONTEXTS_MAX_SIZE, "1000000"));
 
     // Each application will have its own helper context - it is assumed that application
     // names/loaders are unique within each active server instance
-    private static ConcurrentHashMap<Object, ConcurrentHashMap<String, HelperContext>> helperContexts = new ConcurrentHashMap<Object, ConcurrentHashMap<String, HelperContext>>();
+    private static CacheIdentityMap helperContexts = new CacheIdentityMap(helperContextsMaxSize, null, null, false);
     // Each application will have a Map of alias' to identifiers
     private static ConcurrentHashMap<Object, ConcurrentHashMap<String, String>> aliasMap = new ConcurrentHashMap<Object, ConcurrentHashMap<String, String>>();
     // Each application could have separate HelperContextResolver
@@ -521,7 +529,7 @@ static ConcurrentMap<String, HelperContext> getContextMap() {
         ClassLoader appLoader = hCtxMapKey.getLoader();
         // we will use the application name as the map key if set; otherwise we use the loader
         Object contextMapKey = appName != null ? appName : appLoader;
-        ConcurrentHashMap<String, HelperContext> contextMap = helperContexts.get(contextMapKey);
+        ConcurrentHashMap<String, HelperContext> contextMap = (ConcurrentHashMap<String, HelperContext>)helperContexts.get(contextMapKey);
 
         // handle possible redeploy
         // the following block only applies to WAS - hence the loader name check
@@ -542,7 +550,8 @@ static ConcurrentMap<String, HelperContext> getContextMap() {
         if (null == contextMap) {
             contextMap = new ConcurrentHashMap<String, HelperContext>();
             // use putIfAbsent to avoid concurrent entries in the map
-            ConcurrentHashMap<String, HelperContext> existingMap = helperContexts.putIfAbsent(contextMapKey, contextMap);
+            contextMapKey = helperContexts.put(contextMapKey, contextMap, false, 0);
+            ConcurrentHashMap<String, HelperContext> existingMap = (ConcurrentHashMap<String, HelperContext>)helperContexts.get(contextMapKey);
             if (existingMap != null) {
                 // if a new entry was just added, use it instead of the one we just created
                 contextMap = existingMap;
@@ -553,7 +562,7 @@ static ConcurrentMap<String, HelperContext> getContextMap() {
                 if (classLoaderName.contains(WLS_CLASSLOADER_NAME)) {
                     // add a loader/context pair to the helperContexts map to handle case where appName
                     // is no longer available, but the loader from a previous lookup is being used
-                    helperContexts.put(appLoader, contextMap);
+                    helperContexts.put(appLoader, contextMap, false, 0);
                     // add a notification listener to handle redeploy
                     addWLSNotificationListener(appName);
                 } else if (classLoaderName.contains(JBOSS_CLASSLOADER_NAME)) {
@@ -629,6 +638,27 @@ private static boolean removeAppFromMap(Map map, String appName, boolean removeD
         return result;
     }
 
+    /**
+     * Trying to remove entry for a given app the provided map.
+     *
+     * @param map {@link CacheIdentityMap} from which app value should be removed
+     * @param appName application name
+     * @param removeDefaultClassloader whether to try removing the default classloader
+     * @return true if any removal took place
+     */
+    private static boolean removeAppFromMap(CacheIdentityMap map, String appName, boolean removeDefaultClassloader) {
+        boolean result = map.remove(appName, null) != null;
+        // there may be a loader/context pair to remove
+        ClassLoader appLoader = appNameToClassLoaderMap.get(appName);
+        if (appLoader != null) {
+            result |= map.remove(appLoader, null) != null;
+        } else if (removeDefaultClassloader) {
+            // try with Thread ContextClassLoader
+            result |= map.remove(Thread.currentThread().getContextClassLoader(), null) != null;
+        }
+        return result;
+    }
+
     /**
      * INTERNAL:
      * Return the key to be used for Map lookups based in the current thread's
@@ -1010,10 +1040,11 @@ public void makeDefaultContext() {
         ClassLoader appLoader = hCtxMapKey.getLoader();
         Object contextMapKey = appName != null ? appName : appLoader;
 
-        ConcurrentHashMap<String, HelperContext> contexts = helperContexts.get(contextMapKey);
+        ConcurrentHashMap<String, HelperContext> contexts = (ConcurrentHashMap<String, HelperContext>)helperContexts.get(contextMapKey);
         if (contexts == null) {
             contexts = new ConcurrentHashMap<String, HelperContext>();
-            ConcurrentHashMap<String, HelperContext> existingContexts = helperContexts.putIfAbsent(contextMapKey, contexts);
+            contextMapKey = helperContexts.put(contextMapKey, contexts, false, 0);
+            ConcurrentHashMap<String, HelperContext> existingContexts = (ConcurrentHashMap<String, HelperContext>)helperContexts.get(contextMapKey);
             if (existingContexts != null) {
                 contexts = existingContexts;
             } else if (appName != null) {
@@ -1268,7 +1299,7 @@ public static boolean hasHelperContext(String identifier) {
         }
 
         // lastly, check the Map of identifiers to helperContexts
-        ConcurrentHashMap<String, HelperContext> contextMap = helperContexts.get(appKey);
+        ConcurrentHashMap<String, HelperContext> contextMap = (ConcurrentHashMap<String, HelperContext>)helperContexts.get(appKey);
         return (contextMap != null && contextMap.containsKey(id));
     }
 

sdo/org.eclipse.persistence.sdo/src/test/java/org/eclipse/persistence/testing/sdo/helper/SDOHelperContextTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2020 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -17,6 +17,7 @@
 import commonj.sdo.helper.HelperContext;
 import junit.framework.TestCase;
 import org.eclipse.persistence.sdo.helper.SDOHelperContext;
+import static org.eclipse.persistence.sdo.SDOSystemProperties.SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME;
 
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
@@ -39,17 +40,17 @@ private static Map getMap() throws NoSuchFieldException, IllegalAccessException
     protected void setUp() throws Exception {
         super.setUp();
         getMap().clear();
-        strictTypeCheckingPropertyValueBackup = System.getProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME);
-        System.clearProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME);
+        strictTypeCheckingPropertyValueBackup = System.getProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME);
+        System.clearProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME);
     }
 
     @Override
     protected void tearDown() throws Exception {
         super.tearDown();
         if (strictTypeCheckingPropertyValueBackup != null) {
-            System.setProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME, strictTypeCheckingPropertyValueBackup);
+            System.setProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME, strictTypeCheckingPropertyValueBackup);
         } else {
-            System.clearProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME);
+            System.clearProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME);
         }
         strictTypeCheckingPropertyValueBackup = null;
     }
@@ -132,21 +133,21 @@ public void testTypeCheckingStrictnessFlagDefault() {
 
     /**
      * Checks setting {@link SDOHelperContext#isStrictTypeCheckingEnabled()}
-     * using {@link SDOHelperContext#STRICT_TYPE_CHECKING_PROPERTY_NAME} property.
+     * using {@link SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME} property.
      */
     public void testTypeCheckingStrictnessFlagSystemPropertyFalse() {
-        System.setProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME, "false");
+        System.setProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME, "false");
         SDOHelperContext ctx = new SDOHelperContext("testHelperContext");
         assertFalse("Expected value of SDOHelperContext#isStrictTypeCheckingEnabled() is false.",
                 ctx.isStrictTypeCheckingEnabled());
     }
 
     /**
      * Checks setting {@link SDOHelperContext#isStrictTypeCheckingEnabled()}
-     * using {@link SDOHelperContext#STRICT_TYPE_CHECKING_PROPERTY_NAME} property.
+     * using {@link SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME} property.
      */
     public void testTypeCheckingStrictnessFlagSystemPropertyTrue() {
-        System.setProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME, "true");
+        System.setProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME, "true");
         SDOHelperContext ctx = new SDOHelperContext("testHelperContext");
         assertTrue("Expected value of SDOHelperContext#isStrictTypeCheckingEnabled() is true.",
                 ctx.isStrictTypeCheckingEnabled());

sdo/org.eclipse.persistence.sdo/src/test/java/org/eclipse/persistence/testing/sdo/helper/helpercontext/SDOHelperContextTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -29,6 +29,7 @@
 import junit.textui.TestRunner;
 
 import org.eclipse.persistence.dynamic.DynamicClassLoader;
+import org.eclipse.persistence.internal.identitymaps.CacheIdentityMap;
 import org.eclipse.persistence.sdo.SDOProperty;
 import org.eclipse.persistence.sdo.SDOType;
 import org.eclipse.persistence.sdo.helper.SDOHelperContext;
@@ -196,14 +197,14 @@ public void testNotificationListenerWLS() throws Exception {
         helperContextsField.setAccessible(true);
         appNameToClassLoaderMapField.setAccessible(true);
         try {
-            ConcurrentHashMap helperContexts = (ConcurrentHashMap) helperContextsField.get(SDOHelperContext.class);
+            CacheIdentityMap helperContexts = (CacheIdentityMap) helperContextsField.get(SDOHelperContext.class);
             ConcurrentHashMap appNameToClassLoaderMap = (ConcurrentHashMap) appNameToClassLoaderMapField.get(SDOHelperContext.class);
-            final Integer originalHelperContextsSize = helperContexts.size();
+            final Integer originalHelperContextsSize = helperContexts.getSize();
             final Integer originalAppNameToClassLoaderMapSize = appNameToClassLoaderMap.size();
 
             ClassLoader classLoader = new DynamicClassLoader(Thread.currentThread().getContextClassLoader());
-            helperContexts.putIfAbsent(applicationName, contextMap);
-            helperContexts.putIfAbsent(classLoader, contextMap);
+            helperContexts.put(applicationName, contextMap, false, 0);
+            helperContexts.put(classLoader, contextMap, false, 0);
             appNameToClassLoaderMap.put(applicationName, classLoader);
 
             Assert.assertTrue("App1 entry was not added to helperContexts map", helperContexts.containsKey(applicationName));
@@ -227,7 +228,7 @@ public void testNotificationListenerWLS() throws Exception {
             Assert.assertFalse("ClassLoader entry was not added to helperContexts map", helperContexts.containsKey(classLoader));
             Assert.assertFalse("App1 entry was not added to appNameToClassLoaderMapContains map", appNameToClassLoaderMap.containsKey(applicationName));
             Assert.assertEquals("helperContexts map size not restored to original",
-                    originalHelperContextsSize.intValue(), helperContexts.size());
+                    originalHelperContextsSize.intValue(), helperContexts.getSize());
             Assert.assertEquals("appNameToClassLoaderMap size not restored to original",
                     originalAppNameToClassLoaderMapSize.intValue(), appNameToClassLoaderMap.size());
         }

sdo/org.eclipse.persistence.sdo/src/test/java/org/eclipse/persistence/testing/sdo/instanceclass/InstanceClassTestCases.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2020 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -18,11 +18,10 @@
 
 import commonj.sdo.DataObject;
 import commonj.sdo.Type;
-import commonj.sdo.helper.HelperContext;
 
 import org.eclipse.persistence.sdo.helper.SDOHelperContext;
-import org.eclipse.persistence.sequencing.StandardSequence;
 import org.eclipse.persistence.testing.sdo.SDOTestCase;
+import static org.eclipse.persistence.sdo.SDOSystemProperties.SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME;
 
 public class InstanceClassTestCases extends SDOTestCase {
 
@@ -40,17 +39,17 @@ public InstanceClassTestCases(String name) {
     @Override
     public void setUp() {
         super.setUp();
-        strictTypeCheckingPropertyValueBackup = System.getProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME);
-        System.clearProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME);
+        strictTypeCheckingPropertyValueBackup = System.getProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME);
+        System.clearProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME);
     }
 
     @Override
     public void tearDown() throws Exception {
         super.tearDown();
         if (strictTypeCheckingPropertyValueBackup != null) {
-            System.setProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME, strictTypeCheckingPropertyValueBackup);
+            System.setProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME, strictTypeCheckingPropertyValueBackup);
         } else {
-            System.clearProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME);
+            System.clearProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME);
         }
         strictTypeCheckingPropertyValueBackup = null;
     }
@@ -148,7 +147,7 @@ public void testClassWithCorrectGettersAndRelaxedTypeChecking() {
      * This tests works only with {@link SDOHelperContext} and subclasses (otherwise tests nothing).
      */
     public void testInterfaceWithIncorrectGettersAndRelaxedTypeCheckingByProperty() {
-        System.setProperty(SDOHelperContext.STRICT_TYPE_CHECKING_PROPERTY_NAME, "false");
+        System.setProperty(SDO_STRICT_TYPE_CHECKING_PROPERTY_NAME, "false");
         SDOHelperContext helperContext = new SDOHelperContext();
         InputStream xsd = Thread.currentThread().getContextClassLoader().getResourceAsStream(XML_SCHEMA_INTEFACE_INCORRECT_GETTER);
         helperContext.getXSDHelper().define(xsd, null);