Skip to content

Commit

Permalink
[FISH-1018] Out of memory redeploy leaks (#5081)
Browse files Browse the repository at this point in the history
* #4098 Reduced too broad scope of variable

* #4098 Moved field to local variable

- this could be the first cause of leak

* #4098 Refactored inner and anonymous classes to nested static classes

- anonymous and inner holds implicit reference to parent
- this was probably the second cause of leak

* #4098 Fixed code consistency

* Fix more class loader leaks by:
- making sure Server Threads and Timers do not inherit app's context class loaders
- making sure app's security contexts don't get propagated to server threads and timers

Added correct ear classes to class loader leak tests

Co-authored-by: lprimak <[email protected]>
  • Loading branch information
sgflt and lprimak authored Jan 26, 2021
1 parent faf5769 commit bb96e1d
Show file tree
Hide file tree
Showing 14 changed files with 406 additions and 295 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,14 @@
* only if the new code is made subject to such option by the copyright
* holder.
*/
// Portions Copyright [2016-2018] [Payara Foundation and/or its affiliates]
// Portions Copyright [2016-2021] [Payara Foundation and/or its affiliates]

package org.glassfish.concurrent.runtime;

import com.sun.enterprise.config.serverbeans.Applications;
import com.sun.enterprise.container.common.spi.util.ComponentEnvManager;
import com.sun.enterprise.transaction.api.JavaEETransactionManager;
import com.sun.enterprise.util.Utility;
import org.glassfish.api.invocation.InvocationManager;
import org.glassfish.concurrent.LogFacade;
import org.glassfish.concurrent.runtime.deployer.ContextServiceConfig;
Expand All @@ -65,6 +66,7 @@
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.glassfish.enterprise.concurrent.spi.ContextHandle;

/**
* This class provides API to create various Concurrency Utilities objects
Expand Down Expand Up @@ -180,16 +182,16 @@ public void shutdownContextService(String jndiName) {

public synchronized ManagedExecutorServiceImpl getManagedExecutorService(ResourceInfo resource, ManagedExecutorServiceConfig config) {
String jndiName = config.getJndiName();

if (managedExecutorServiceMap != null && managedExecutorServiceMap.containsKey(jndiName)) {
return managedExecutorServiceMap.get(jndiName);
}
ManagedThreadFactoryImpl managedThreadFactory = new ManagedThreadFactoryImpl(

ManagedThreadFactoryImpl managedThreadFactory = new ThreadFactoryWrapper(
config.getJndiName() + "-managedThreadFactory",
null,
config.getThreadPriority());

ManagedExecutorServiceImpl mes = new ManagedExecutorServiceImpl(config.getJndiName(),
managedThreadFactory,
config.getHungAfterSeconds() * 1000L, // in millseconds
Expand All @@ -202,17 +204,17 @@ public synchronized ManagedExecutorServiceImpl getManagedExecutorService(Resourc
createContextService(config.getJndiName() + "-contextservice",
config.getContextInfo(), config.getContextInfoEnabled(), true),
AbstractManagedExecutorService.RejectPolicy.ABORT);

if (managedExecutorServiceMap == null) {
managedExecutorServiceMap = new HashMap();
}

managedExecutorServiceMap.put(jndiName, mes);

if (config.getHungAfterSeconds() > 0L && !config.isLongRunningTasks()) {
scheduleInternalTimer();
}

return mes;
}

Expand All @@ -234,7 +236,7 @@ public synchronized ManagedScheduledExecutorServiceImpl getManagedScheduledExecu
if (managedScheduledExecutorServiceMap != null && managedScheduledExecutorServiceMap.containsKey(jndiName)) {
return managedScheduledExecutorServiceMap.get(jndiName);
}
ManagedThreadFactoryImpl managedThreadFactory = new ManagedThreadFactoryImpl(
ManagedThreadFactoryImpl managedThreadFactory = new ThreadFactoryWrapper(
config.getJndiName() + "-managedThreadFactory",
null,
config.getThreadPriority());
Expand Down Expand Up @@ -275,7 +277,7 @@ public synchronized ManagedThreadFactoryImpl getManagedThreadFactory(ResourceInf
if (managedThreadFactoryMap != null && managedThreadFactoryMap.containsKey(jndiName)) {
return managedThreadFactoryMap.get(jndiName);
}
ManagedThreadFactoryImpl managedThreadFactory = new ManagedThreadFactoryImpl(config.getJndiName(),
ManagedThreadFactoryImpl managedThreadFactory = new ThreadFactoryWrapper(config.getJndiName(),
createContextService(config.getJndiName() + "-contextservice",
config.getContextInfo(), config.getContextInfoEnabled(), true),
config.getThreadPriority());
Expand Down Expand Up @@ -360,7 +362,7 @@ private ContextSetupProviderImpl.CONTEXT_TYPE[] parseContextInfo(String contextI
private void scheduleInternalTimer() {
if (internalScheduler == null) {
String name = "glassfish-internal";
ManagedThreadFactoryImpl managedThreadFactory = new ManagedThreadFactoryImpl(
ManagedThreadFactoryImpl managedThreadFactory = new ThreadFactoryWrapper(
name + "-managedThreadFactory",
null,
Thread.NORM_PRIORITY);
Expand All @@ -378,6 +380,26 @@ private void scheduleInternalTimer() {
}
}

/**
* context loader propagation to threads causes memory leaks on redeploy
*/
private static final class ThreadFactoryWrapper extends ManagedThreadFactoryImpl {
public ThreadFactoryWrapper(String string, ContextServiceImpl contextService, int threadPriority) {
super(string, contextService, threadPriority);
}

@Override
protected AbstractManagedThread createThread(Runnable r, ContextHandle contextHandleForSetup) {
ClassLoader appClassLoader = Utility.getClassLoader();
Utility.setContextClassLoader(null);
try {
return super.createThread(r, contextHandleForSetup);
} finally {
Utility.setContextClassLoader(appClassLoader);
}
}
}

@Override
public void postConstruct() {
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
* only if the new code is made subject to such option by the copyright
* holder.
*/
// Portions Copyright [2019] [Payara Foundation and/or its affiliates]
// Portions Copyright [2019-2021] [Payara Foundation and/or its affiliates]

package com.sun.enterprise.connectors;

Expand All @@ -63,6 +63,7 @@
import com.sun.enterprise.security.SecurityServicesUtil;
import com.sun.enterprise.security.jaspic.callback.ContainerCallbackHandler;
import com.sun.enterprise.transaction.api.JavaEETransactionManager;
import com.sun.enterprise.util.Utility;
import com.sun.logging.LogDomains;
import org.glassfish.admin.monitor.MonitoringBootstrap;
import org.glassfish.api.admin.ProcessEnvironment;
Expand Down Expand Up @@ -106,6 +107,8 @@
import javax.transaction.Transaction;
import java.io.PrintWriter;
import java.net.URI;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.sql.Connection;
import java.sql.SQLException;
import java.util.*;
Expand Down Expand Up @@ -886,7 +889,7 @@ public void postConstruct() {
}
if(isServer() || isEmbedded()){
poolMonitoringLevelListener = poolMonitoringLevelListenerProvider.get();

// Force initialization of the ResourceManager
getResourceManager();
}
Expand Down Expand Up @@ -968,7 +971,7 @@ public void createConnectorConnectionPool(ConnectorConnectionPool ccp,
throws ConnectorRuntimeException {
ccPoolAdmService.createConnectorConnectionPool(ccp, connectionDefinitionName, rarName, props, securityMaps);
}

private synchronized org.glassfish.resourcebase.resources.listener.ResourceManager getResourceManager() {
if (resourceManager == null) {
try {
Expand All @@ -978,7 +981,7 @@ private synchronized org.glassfish.resourcebase.resources.listener.ResourceManag
return null;
}
}

return resourceManager;
}

Expand Down Expand Up @@ -1027,7 +1030,14 @@ public InvocationManager getInvocationManager() {
}

public Timer getTimer() {
return ConnectorTimerProxy.getProxy();
ClassLoader appClassLoader = Utility.getClassLoader();
// prevent app class loader leaking into timer
Utility.setContextClassLoader(null);
try {
return AccessController.doPrivileged((PrivilegedAction<Timer>) ConnectorTimerProxy::getProxy);
} finally {
Utility.setContextClassLoader(appClassLoader);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
* only if the new code is made subject to such option by the copyright
* holder.
*/
// Portions Copyright [2016] [Payara Foundation and/or its affiliates]
// Portions Copyright [2016-2021] [Payara Foundation and/or its affiliates]

package org.glassfish.javaee.full.deployment;

Expand All @@ -47,6 +47,7 @@

import com.sun.enterprise.loader.ASURLClassLoader;
import java.util.logging.Logger;
import org.glassfish.common.util.InstanceCounter;
import org.glassfish.internal.api.DelegatingClassLoader;
import org.glassfish.hk2.api.PreDestroy;

Expand All @@ -62,6 +63,7 @@ public class EarClassLoader extends ASURLClassLoader
boolean isPreDestroyCalled = false;
private static final Logger log = Logger.getLogger(EarClassLoader.class.getName());
private final Application application;
private final InstanceCounter instanceCounter = new InstanceCounter(this);

public EarClassLoader(ClassLoader classLoader, Application application) {
super(classLoader);
Expand Down Expand Up @@ -90,13 +92,13 @@ public void preDestroy() {
try {
for (ClassLoaderHolder clh : moduleClassLoaders) {
// destroy all the module classloaders
if ( !(clh.loader instanceof EarLibClassLoader) &&
!(clh.loader instanceof EarClassLoader) &&
if ( !(clh.loader instanceof EarLibClassLoader) &&
!(clh.loader instanceof EarClassLoader) &&
!isRARCL(clh.loader)) {
try {
PreDestroy.class.cast(clh.loader).preDestroy();
} catch (Exception e) {
// ignore, the class loader does not need to be
// ignore, the class loader does not need to be
// explicitly stopped.
}
}
Expand All @@ -111,7 +113,7 @@ public void preDestroy() {
try {
PreDestroy.class.cast(cf).preDestroy();
} catch (Exception e) {
// ignore, the class loader does not need to be
// ignore, the class loader does not need to be
// explicitly stopped.
}
}
Expand Down
Loading

0 comments on commit bb96e1d

Please sign in to comment.