From 38c8d9f1a98de11bd4889975e77a0e32e5e4a355 Mon Sep 17 00:00:00 2001 From: Ryan Dens Date: Sun, 21 Mar 2021 17:37:25 -0400 Subject: [PATCH 1/5] :sparkles: create memoizing AgentConfigSupplier encapsulating double-checked locking logic --- .../core/config/AgentConfigSupplier.java | 111 ++++++++++++++++++ .../core/config/AgentConfigSupplierTest.java | 100 ++++++++++++++++ 2 files changed, 211 insertions(+) create mode 100644 javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java create mode 100644 javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java diff --git a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java new file mode 100644 index 000000000..f4bef4252 --- /dev/null +++ b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java @@ -0,0 +1,111 @@ +/* + * Copyright The Hypertrace Authors + * + * Licensed 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.hypertrace.agent.core.config; + +import com.google.common.annotations.VisibleForTesting; +import com.google.protobuf.InvalidProtocolBufferException; +import com.google.protobuf.util.JsonFormat; +import java.util.function.Supplier; +import org.hypertrace.agent.config.Config; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Memoizing supplier inspired by Guava's NonSerializableMemoizingSupplier. + * Decorates a {@link Supplier} of {@link Config.AgentConfig} with memoizing, logging, and + * error-handling logic for a consistent once-per Java process Config initialization. + * + *

Thread-safe + */ +public final class AgentConfigSupplier implements Supplier { + + private static final Logger log = LoggerFactory.getLogger(AgentConfigSupplier.class); + + private final Supplier supplier; + private volatile boolean initialized; + /** + * Although not explicitly volatile, this field is effectively volatile because updates to this + * field are always followed by updates to the volatile field {@link #initialized}. In {@link + * AgentConfigSupplier#get()} due to the semantics of how the volatile keyword is implemented. + * Namely, anything (not just volatile fields) that was visible to thread A when it writes to + * volatile field {@link #initialized} becomes visible to thread B when it reads {@link + * #initialized}. + */ + private Config.AgentConfig value; + + /** + * @param supplier inner supplier of {@link Config.AgentConfig} to decorate with config loading + * logic + */ + public AgentConfigSupplier(final Supplier supplier) { + this.supplier = supplier; + } + + /** + * Delegates to an inner composed {@link Supplier} the first time this method is invoked by any + * thread, and caches the value in the effectively volatile field {@link #value}. On subsequent + * invocations, the {@link #value} will be returned without invocation of the {@link #supplier}, + * unless {@link #reset()} has been invoked since the first invocation of this method. + * + * @return the {@link Config.AgentConfig} singleton. + */ + @Override + public Config.AgentConfig get() { + if (!initialized) { + synchronized (this) { + if (!initialized) { + final Config.AgentConfig result = supplier.get(); + value = result; + initialized = true; + try { + log.info( + "Config loaded: {}", + JsonFormat.printer().omittingInsignificantWhitespace().print(result)); + } catch (InvalidProtocolBufferException e) { + throw new RuntimeException("Error serializing protobuf config for writing to log", e); + } + return result; + } + } + } + return value; + } + + /** + * Resets the memoizing aspect of this {@link Supplier}, forcing the invocation of the inner + * {@link #supplier} once again. Because this method exists, we cannot dereference the {@link + * #supplier} field to make it eligible for GC as done by the Gauva {@code + * NonSerializableMemoizingSupplier}. + * + *

There's no "double-checked locking" optimization to make here. In the case of resetting, we + * might consider checking the volatile field {@link #initialized} prior to acquiring the lock on + * the instance and short-circuiting if {@link #initialized} is true. However, this API is only + * visible for testing purposes and we only expect it to be called after the {@link + * Config.AgentConfig} is loaded. So we simply want to make sure this class is thread-safe by + * making sure updates to the {@link #initialized} and {@link #value} field are atomic + */ + @VisibleForTesting + void reset() { + synchronized (this) { + if (initialized) { + value = null; + initialized = false; + } + } + } +} diff --git a/javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java b/javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java new file mode 100644 index 000000000..95e8b3539 --- /dev/null +++ b/javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java @@ -0,0 +1,100 @@ +/* + * Copyright The Hypertrace Authors + * + * Licensed 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.hypertrace.agent.core.config; + +import static org.junit.jupiter.api.Assertions.*; + +import java.io.IOException; +import java.net.URL; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; +import org.hypertrace.agent.config.Config; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +final class AgentConfigSupplierTest { + + private CountingAgentConfigSupplier inner; + private AgentConfigSupplier agentConfigSupplier; + + @BeforeEach + void beforeEach() { + // GIVEN a supplier of AgentConfig that also counts how many time its been invoked + inner = new CountingAgentConfigSupplier(); + // WHEN we decorate it with the AgentConfigSupplier + agentConfigSupplier = new AgentConfigSupplier(inner); + // VERIFY starts out reporting its been invoked 0 times + assertEquals(0, inner.getCount()); + } + + @Test + void testMemoizingAgentConfig() { + // WHEN we access the config + final Config.AgentConfig agentConfig = agentConfigSupplier.get(); + // VERIFY it has been invoked once + assertEquals(1, inner.getCount()); + + // WHEN we access the config + final Config.AgentConfig secondConfig = agentConfigSupplier.get(); + // VERIFY the inner supplier has still only been invoked once + assertEquals(1, inner.getCount()); + // VERIFY the two configs are the same instance + assertSame(agentConfig, secondConfig); + } + + @Test + void testResetAgentConfig() { + // WHEN we access the config + final Config.AgentConfig agentConfig = agentConfigSupplier.get(); + // VERIFY it has been invoked once + assertEquals(1, inner.getCount()); + + // WHEN we reset the supplier + agentConfigSupplier.reset(); + // AND WHEN we access the config again + final Config.AgentConfig secondConfig = agentConfigSupplier.get(); + + // VERIFY it has been invoked twice + assertEquals(2, inner.getCount()); + // VERIFY the two configs are different instances + assertNotSame(agentConfig, secondConfig); + } + + /** + * thread-safe {@link Config.AgentConfig} {@link Supplier} that also counts how many times it has + * been invoked + */ + private static final class CountingAgentConfigSupplier implements Supplier { + + private final AtomicInteger count = new AtomicInteger(0); + + @Override + public Config.AgentConfig get() { + count.getAndIncrement(); + URL resource = getClass().getClassLoader().getResource("emptyconfig.yaml"); + try { + return HypertraceConfig.load(resource.getPath()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + int getCount() { + return count.get(); + } + } +} From 49bd84b212edbf99dbb75c13eabc9cb2f36b1acf Mon Sep 17 00:00:00 2001 From: Ryan Dens Date: Sun, 21 Mar 2021 17:41:48 -0400 Subject: [PATCH 2/5] :recycle: refactor HypertraceConfig to use thread-safe AgentConfigSupplier --- .../agent/core/config/HypertraceConfig.java | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java index 0e8c9ea49..20bd1622f 100644 --- a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java +++ b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java @@ -51,7 +51,8 @@ private HypertraceConfig() {} // so avoiding for perf reasons private static volatile boolean servletCausingException; - private static AgentConfig agentConfig; + private static final AgentConfigSupplier agentConfigSupplier = + new AgentConfigSupplier(HypertraceConfig::load); static final String DEFAULT_SERVICE_NAME = "unknown"; static final String DEFAULT_REPORTING_ENDPOINT = "http://localhost:9411/api/v2/spans"; @@ -61,21 +62,7 @@ private HypertraceConfig() {} static final int DEFAULT_BODY_MAX_SIZE_BYTES = 128 * 1024; public static AgentConfig get() { - if (agentConfig == null) { - synchronized (HypertraceConfig.class) { - if (agentConfig == null) { - try { - agentConfig = load(); - log.info( - "Config loaded: {}", - JsonFormat.printer().omittingInsignificantWhitespace().print(agentConfig)); - } catch (IOException e) { - throw new RuntimeException("Could not load config", e); - } - } - } - } - return agentConfig; + return agentConfigSupplier.get(); } public static boolean isInstrumentationEnabled(String primaryName, String[] otherNames) { @@ -123,16 +110,20 @@ private static boolean isHypertraceType(String message) { /** Reset the config, use only in tests. */ @VisibleForTesting public static void reset() { - agentConfig = null; + agentConfigSupplier.reset(); } - private static AgentConfig load() throws IOException { + private static AgentConfig load() { String configFile = EnvironmentConfig.getProperty(EnvironmentConfig.CONFIG_FILE_PROPERTY); if (configFile == null) { return EnvironmentConfig.applyPropertiesAndEnvVars(applyDefaults(AgentConfig.newBuilder())) .build(); } - return load(configFile); + try { + return load(configFile); + } catch (IOException e) { + throw new RuntimeException("Could not load config", e); + } } @VisibleForTesting From ca8f431c318fc42cab7c95f65c11e33eac234669 Mon Sep 17 00:00:00 2001 From: Ryan Dens Date: Sat, 27 Mar 2021 11:15:25 -0400 Subject: [PATCH 3/5] Revert ":recycle: refactor HypertraceConfig to use thread-safe AgentConfigSupplier" This reverts commit 19ad84b7c4183a11977a1dce69a03c92c0211336. --- .../agent/core/config/HypertraceConfig.java | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java index 20bd1622f..0e8c9ea49 100644 --- a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java +++ b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java @@ -51,8 +51,7 @@ private HypertraceConfig() {} // so avoiding for perf reasons private static volatile boolean servletCausingException; - private static final AgentConfigSupplier agentConfigSupplier = - new AgentConfigSupplier(HypertraceConfig::load); + private static AgentConfig agentConfig; static final String DEFAULT_SERVICE_NAME = "unknown"; static final String DEFAULT_REPORTING_ENDPOINT = "http://localhost:9411/api/v2/spans"; @@ -62,7 +61,21 @@ private HypertraceConfig() {} static final int DEFAULT_BODY_MAX_SIZE_BYTES = 128 * 1024; public static AgentConfig get() { - return agentConfigSupplier.get(); + if (agentConfig == null) { + synchronized (HypertraceConfig.class) { + if (agentConfig == null) { + try { + agentConfig = load(); + log.info( + "Config loaded: {}", + JsonFormat.printer().omittingInsignificantWhitespace().print(agentConfig)); + } catch (IOException e) { + throw new RuntimeException("Could not load config", e); + } + } + } + } + return agentConfig; } public static boolean isInstrumentationEnabled(String primaryName, String[] otherNames) { @@ -110,20 +123,16 @@ private static boolean isHypertraceType(String message) { /** Reset the config, use only in tests. */ @VisibleForTesting public static void reset() { - agentConfigSupplier.reset(); + agentConfig = null; } - private static AgentConfig load() { + private static AgentConfig load() throws IOException { String configFile = EnvironmentConfig.getProperty(EnvironmentConfig.CONFIG_FILE_PROPERTY); if (configFile == null) { return EnvironmentConfig.applyPropertiesAndEnvVars(applyDefaults(AgentConfig.newBuilder())) .build(); } - try { - return load(configFile); - } catch (IOException e) { - throw new RuntimeException("Could not load config", e); - } + return load(configFile); } @VisibleForTesting From da325bdc5944d8097a0a2077653e373ac1f9600c Mon Sep 17 00:00:00 2001 From: Ryan Dens Date: Sat, 27 Mar 2021 11:23:29 -0400 Subject: [PATCH 4/5] :ok_hand: move double-checked locking implementation back into HypertraceConfig --- .../org/hypertrace/agent/core/config/HypertraceConfig.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java index 0e8c9ea49..2e137b3fa 100644 --- a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java +++ b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/HypertraceConfig.java @@ -51,7 +51,8 @@ private HypertraceConfig() {} // so avoiding for perf reasons private static volatile boolean servletCausingException; - private static AgentConfig agentConfig; + // volatile field in order to properly handle lazy initialization with double-checked locking + private static volatile AgentConfig agentConfig; static final String DEFAULT_SERVICE_NAME = "unknown"; static final String DEFAULT_REPORTING_ENDPOINT = "http://localhost:9411/api/v2/spans"; @@ -123,7 +124,9 @@ private static boolean isHypertraceType(String message) { /** Reset the config, use only in tests. */ @VisibleForTesting public static void reset() { - agentConfig = null; + synchronized (HypertraceConfig.class) { + agentConfig = null; + } } private static AgentConfig load() throws IOException { From db27d36e1f353fc45c83c7a8af6f3e1e43bd0dc6 Mon Sep 17 00:00:00 2001 From: Ryan Dens Date: Sat, 27 Mar 2021 11:25:25 -0400 Subject: [PATCH 5/5] :fire: delete AgentConfigSupplier as it is now unused --- .../core/config/AgentConfigSupplier.java | 111 ------------------ .../core/config/AgentConfigSupplierTest.java | 100 ---------------- 2 files changed, 211 deletions(-) delete mode 100644 javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java delete mode 100644 javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java diff --git a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java b/javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java deleted file mode 100644 index f4bef4252..000000000 --- a/javaagent-core/src/main/java/org/hypertrace/agent/core/config/AgentConfigSupplier.java +++ /dev/null @@ -1,111 +0,0 @@ -/* - * Copyright The Hypertrace Authors - * - * Licensed 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.hypertrace.agent.core.config; - -import com.google.common.annotations.VisibleForTesting; -import com.google.protobuf.InvalidProtocolBufferException; -import com.google.protobuf.util.JsonFormat; -import java.util.function.Supplier; -import org.hypertrace.agent.config.Config; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * Memoizing supplier inspired by Guava's NonSerializableMemoizingSupplier. - * Decorates a {@link Supplier} of {@link Config.AgentConfig} with memoizing, logging, and - * error-handling logic for a consistent once-per Java process Config initialization. - * - *

Thread-safe - */ -public final class AgentConfigSupplier implements Supplier { - - private static final Logger log = LoggerFactory.getLogger(AgentConfigSupplier.class); - - private final Supplier supplier; - private volatile boolean initialized; - /** - * Although not explicitly volatile, this field is effectively volatile because updates to this - * field are always followed by updates to the volatile field {@link #initialized}. In {@link - * AgentConfigSupplier#get()} due to the semantics of how the volatile keyword is implemented. - * Namely, anything (not just volatile fields) that was visible to thread A when it writes to - * volatile field {@link #initialized} becomes visible to thread B when it reads {@link - * #initialized}. - */ - private Config.AgentConfig value; - - /** - * @param supplier inner supplier of {@link Config.AgentConfig} to decorate with config loading - * logic - */ - public AgentConfigSupplier(final Supplier supplier) { - this.supplier = supplier; - } - - /** - * Delegates to an inner composed {@link Supplier} the first time this method is invoked by any - * thread, and caches the value in the effectively volatile field {@link #value}. On subsequent - * invocations, the {@link #value} will be returned without invocation of the {@link #supplier}, - * unless {@link #reset()} has been invoked since the first invocation of this method. - * - * @return the {@link Config.AgentConfig} singleton. - */ - @Override - public Config.AgentConfig get() { - if (!initialized) { - synchronized (this) { - if (!initialized) { - final Config.AgentConfig result = supplier.get(); - value = result; - initialized = true; - try { - log.info( - "Config loaded: {}", - JsonFormat.printer().omittingInsignificantWhitespace().print(result)); - } catch (InvalidProtocolBufferException e) { - throw new RuntimeException("Error serializing protobuf config for writing to log", e); - } - return result; - } - } - } - return value; - } - - /** - * Resets the memoizing aspect of this {@link Supplier}, forcing the invocation of the inner - * {@link #supplier} once again. Because this method exists, we cannot dereference the {@link - * #supplier} field to make it eligible for GC as done by the Gauva {@code - * NonSerializableMemoizingSupplier}. - * - *

There's no "double-checked locking" optimization to make here. In the case of resetting, we - * might consider checking the volatile field {@link #initialized} prior to acquiring the lock on - * the instance and short-circuiting if {@link #initialized} is true. However, this API is only - * visible for testing purposes and we only expect it to be called after the {@link - * Config.AgentConfig} is loaded. So we simply want to make sure this class is thread-safe by - * making sure updates to the {@link #initialized} and {@link #value} field are atomic - */ - @VisibleForTesting - void reset() { - synchronized (this) { - if (initialized) { - value = null; - initialized = false; - } - } - } -} diff --git a/javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java b/javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java deleted file mode 100644 index 95e8b3539..000000000 --- a/javaagent-core/src/test/java/org/hypertrace/agent/core/config/AgentConfigSupplierTest.java +++ /dev/null @@ -1,100 +0,0 @@ -/* - * Copyright The Hypertrace Authors - * - * Licensed 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.hypertrace.agent.core.config; - -import static org.junit.jupiter.api.Assertions.*; - -import java.io.IOException; -import java.net.URL; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Supplier; -import org.hypertrace.agent.config.Config; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -final class AgentConfigSupplierTest { - - private CountingAgentConfigSupplier inner; - private AgentConfigSupplier agentConfigSupplier; - - @BeforeEach - void beforeEach() { - // GIVEN a supplier of AgentConfig that also counts how many time its been invoked - inner = new CountingAgentConfigSupplier(); - // WHEN we decorate it with the AgentConfigSupplier - agentConfigSupplier = new AgentConfigSupplier(inner); - // VERIFY starts out reporting its been invoked 0 times - assertEquals(0, inner.getCount()); - } - - @Test - void testMemoizingAgentConfig() { - // WHEN we access the config - final Config.AgentConfig agentConfig = agentConfigSupplier.get(); - // VERIFY it has been invoked once - assertEquals(1, inner.getCount()); - - // WHEN we access the config - final Config.AgentConfig secondConfig = agentConfigSupplier.get(); - // VERIFY the inner supplier has still only been invoked once - assertEquals(1, inner.getCount()); - // VERIFY the two configs are the same instance - assertSame(agentConfig, secondConfig); - } - - @Test - void testResetAgentConfig() { - // WHEN we access the config - final Config.AgentConfig agentConfig = agentConfigSupplier.get(); - // VERIFY it has been invoked once - assertEquals(1, inner.getCount()); - - // WHEN we reset the supplier - agentConfigSupplier.reset(); - // AND WHEN we access the config again - final Config.AgentConfig secondConfig = agentConfigSupplier.get(); - - // VERIFY it has been invoked twice - assertEquals(2, inner.getCount()); - // VERIFY the two configs are different instances - assertNotSame(agentConfig, secondConfig); - } - - /** - * thread-safe {@link Config.AgentConfig} {@link Supplier} that also counts how many times it has - * been invoked - */ - private static final class CountingAgentConfigSupplier implements Supplier { - - private final AtomicInteger count = new AtomicInteger(0); - - @Override - public Config.AgentConfig get() { - count.getAndIncrement(); - URL resource = getClass().getClassLoader().getResource("emptyconfig.yaml"); - try { - return HypertraceConfig.load(resource.getPath()); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - - int getCount() { - return count.get(); - } - } -}