From 2f8762fd9d700eb1fd2d44bc8893e5240ddd5893 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 19 Aug 2019 08:05:02 +0300 Subject: [PATCH 1/4] Setting JDBC statement map cleaner thread ccl to null --- .../java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java index 1ca52c3ca1..309d6b275f 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java @@ -35,7 +35,12 @@ public abstract class JdbcHelper { @SuppressWarnings("WeakerAccess") @VisibleForAdvice - public static final WeakConcurrentMap statementSqlMap = new WeakConcurrentMap<>(true); + public static final WeakConcurrentMap statementSqlMap; + + static { + statementSqlMap = new WeakConcurrentMap<>(true); + statementSqlMap.getCleanerThread().setContextClassLoader(null); + } /** * Maps the provided sql to the provided Statement object From 1137bd43abcb54773e36101ee64ed10fa0cf4e16 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 19 Aug 2019 12:42:42 +0300 Subject: [PATCH 2/4] Add to CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c39078632f..06387166a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ This makes the runtime attachment work in more environments such as minimal Docker containers. Note that the runtime attachment currently does not work for OSGi containers like those used in many application servers such as JBoss and WildFly. See the [documentation](https://www.elastic.co/guide/en/apm/agent/java/master/setup-attach-cli.html) for more information. + * JDBC statement map is leaking in Tomcat if the application that first used it is udeployed/redeployed. See [this + related discussion](https://discuss.elastic.co/t/elastic-apm-agent-jdbchelper-seems-to-use-a-lot-of-memory/195295). # Breaking Changes * The `apm-agent-attach.jar` is not executable anymore. From 6ba8eecf09101ef19c189c91b05f2d4f61c1b3f8 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 19 Aug 2019 12:45:57 +0300 Subject: [PATCH 3/4] Restoring the comment... --- .../java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java index 309d6b275f..645f962405 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java @@ -39,6 +39,11 @@ public abstract class JdbcHelper { static { statementSqlMap = new WeakConcurrentMap<>(true); + // This class will probably be loaded in the context of a request processing thread + // whose context class loader is the web application ClassLoader. + // This leaks the web application ClassLoader when the application is undeployed/redeployed. + // Tomcat will then stop the thread because it thinks it was created by the web application. + // That means that the map will never be cleared, creating a severe memory leak. statementSqlMap.getCleanerThread().setContextClassLoader(null); } From 4c39fb62653c6958b6a09aa344e2d42f24c21f91 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 21 Aug 2019 09:56:52 +0300 Subject: [PATCH 4/4] Change WeakConcurrentMap's cleaner thread's CCL centrally --- .../apm/agent/util/DataStructures.java | 48 +++++++++++++++++++ .../apm/agent/jdbc/helper/JdbcHelper.java | 13 +---- .../apm/agent/jdbc/helper/JdbcHelperImpl.java | 3 +- .../HttpUrlConnectionInstrumentation.java | 3 +- 4 files changed, 54 insertions(+), 13 deletions(-) create mode 100644 apm-agent-core/src/main/java/co/elastic/apm/agent/util/DataStructures.java diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/util/DataStructures.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/util/DataStructures.java new file mode 100644 index 0000000000..f80928901e --- /dev/null +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/util/DataStructures.java @@ -0,0 +1,48 @@ +/*- + * #%L + * Elastic APM Java agent + * %% + * Copyright (C) 2018 - 2019 Elastic and contributors + * %% + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. 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. + * #L% + */ +package co.elastic.apm.agent.util; + +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; + +public class DataStructures { + + /** + * Use this utility method for WeakConcurrentMap creation if it is created in the context of a request processing + * thread whose context class loader is the web application ClassLoader. + * This leaks the web application ClassLoader when the application is undeployed/redeployed. + *

+ * Tomcat will then stop the thread because it thinks it was created by the web application. + * That means that the map will never be cleared, creating a severe memory leak. + * + * @param map key type + * @param map value type + * @return a new WeakConcurrentMap with a cleaner thread who's context class loader is the system/bootstrap class loader + */ + public static WeakConcurrentMap createWeakConcurrentMapWithCleanerThread() { + WeakConcurrentMap map = new WeakConcurrentMap<>(true); + map.getCleanerThread().setContextClassLoader(null); + return map; + } +} diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java index 645f962405..29f736bfba 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java @@ -27,6 +27,7 @@ import co.elastic.apm.agent.bci.VisibleForAdvice; import co.elastic.apm.agent.impl.transaction.Span; import co.elastic.apm.agent.impl.transaction.TraceContextHolder; +import co.elastic.apm.agent.util.DataStructures; import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import javax.annotation.Nullable; @@ -35,17 +36,7 @@ public abstract class JdbcHelper { @SuppressWarnings("WeakerAccess") @VisibleForAdvice - public static final WeakConcurrentMap statementSqlMap; - - static { - statementSqlMap = new WeakConcurrentMap<>(true); - // This class will probably be loaded in the context of a request processing thread - // whose context class loader is the web application ClassLoader. - // This leaks the web application ClassLoader when the application is undeployed/redeployed. - // Tomcat will then stop the thread because it thinks it was created by the web application. - // That means that the map will never be cleared, creating a severe memory leak. - statementSqlMap.getCleanerThread().setContextClassLoader(null); - } + public static final WeakConcurrentMap statementSqlMap = DataStructures.createWeakConcurrentMapWithCleanerThread(); /** * Maps the provided sql to the provided Statement object diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java index 4ea4a2795c..6ea25e5054 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java @@ -29,6 +29,7 @@ import co.elastic.apm.agent.impl.transaction.Span; import co.elastic.apm.agent.impl.transaction.TraceContextHolder; import co.elastic.apm.agent.jdbc.signature.SignatureParser; +import co.elastic.apm.agent.util.DataStructures; import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -43,7 +44,7 @@ public class JdbcHelperImpl extends JdbcHelper { public static final String DB_SPAN_ACTION = "query"; private static final Logger logger = LoggerFactory.getLogger(JdbcHelperImpl.class); - private static final WeakConcurrentMap metaDataMap = new WeakConcurrentMap(true); + private static final WeakConcurrentMap metaDataMap = DataStructures.createWeakConcurrentMapWithCleanerThread(); @VisibleForAdvice public static final ThreadLocal SIGNATURE_PARSER_THREAD_LOCAL = new ThreadLocal() { diff --git a/apm-agent-plugins/apm-urlconnection-plugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java b/apm-agent-plugins/apm-urlconnection-plugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java index 2e25d09041..c0f25d8a9a 100644 --- a/apm-agent-plugins/apm-urlconnection-plugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java +++ b/apm-agent-plugins/apm-urlconnection-plugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java @@ -29,6 +29,7 @@ import co.elastic.apm.agent.http.client.HttpClientHelper; import co.elastic.apm.agent.impl.transaction.Span; import co.elastic.apm.agent.impl.transaction.TraceContext; +import co.elastic.apm.agent.util.DataStructures; import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.NamedElement; @@ -51,7 +52,7 @@ public abstract class HttpUrlConnectionInstrumentation extends ElasticApmInstrumentation { @VisibleForAdvice - public static final WeakConcurrentMap inFlightSpans = new WeakConcurrentMap(true); + public static final WeakConcurrentMap inFlightSpans = DataStructures.createWeakConcurrentMapWithCleanerThread(); @Override public Collection getInstrumentationGroupNames() {