From 364675f5ab30f0fcc7a9bd0d81b191c06487fa87 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Wed, 3 Aug 2022 14:39:48 -0400 Subject: [PATCH 1/3] move allowances Map from Feline class to avoid possible deadlock --- .../spotify/feline/AllowancesTransformer.java | 25 ++++++++++++++++--- .../main/java/com/spotify/feline/Feline.java | 16 +++--------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java b/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java index 43db469..dbff72b 100644 --- a/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java +++ b/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java @@ -17,8 +17,11 @@ package com.spotify.feline; +import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; @@ -31,10 +34,26 @@ */ class AllowancesTransformer implements AgentBuilder.Transformer { - private final Map> allowances; + private final ConcurrentMap> allowances; - AllowancesTransformer(Map> allowances) { - this.allowances = allowances; + AllowancesTransformer() { + this.allowances = new ConcurrentHashMap<>(); + } + + void allow(final String className, final String methodName) { + allowances.compute( + className, + (key, allowances) -> { + if (allowances == null) { + allowances = new HashSet<>(); + } + allowances.add(methodName); + return allowances; + }); + } + + boolean containsClass(String name) { + return allowances.containsKey(name); } @Override diff --git a/feline/src/main/java/com/spotify/feline/Feline.java b/feline/src/main/java/com/spotify/feline/Feline.java index 20943cf..8f27e15 100644 --- a/feline/src/main/java/com/spotify/feline/Feline.java +++ b/feline/src/main/java/com/spotify/feline/Feline.java @@ -32,7 +32,7 @@ /** Detects blocking calls to @link CompletableFuture and notifies registered consumers. */ public class Feline { - private static final Map> allowances = new ConcurrentHashMap<>(); + private static final AllowancesTransformer allowancesTransformer = new AllowancesTransformer(); /** * Registers a consumer that will be invoked when blocking calls are detected. Consumers can throw @@ -152,15 +152,7 @@ public static boolean removeOnExitConsumer( * @param methodName a method name */ public static void allowBlockingCallsInside(final String className, final String methodName) { - allowances.compute( - className, - (key, allowances) -> { - if (allowances == null) { - allowances = new HashSet<>(); - } - allowances.add(methodName); - return allowances; - }); + allowancesTransformer.allow(className, methodName); } /** @@ -214,8 +206,8 @@ public static boolean removeThreadLocalInitialValueConsumer(final Runnable consu .asTerminalTransformation() // Instrument allowed/disallowed methods - .type(it -> allowances.containsKey(it.getName())) - .transform(new AllowancesTransformer(allowances)) + .type(it -> allowancesTransformer.containsClass(it.getName())) + .transform(allowancesTransformer) .asTerminalTransformation() // instrument ThreadLocal From e1af6f9856f9b9c083ff7e7598c642230f3f35a8 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Wed, 3 Aug 2022 15:06:15 -0400 Subject: [PATCH 2/3] implement ElementMatcher in AllowancesTransformer --- .../java/com/spotify/feline/AllowancesTransformer.java | 9 +++++---- feline/src/main/java/com/spotify/feline/Feline.java | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java b/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java index dbff72b..d162c50 100644 --- a/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java +++ b/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java @@ -18,7 +18,6 @@ package com.spotify.feline; import java.util.HashSet; -import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -26,13 +25,14 @@ import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.DynamicType; +import net.bytebuddy.matcher.ElementMatcher; import net.bytebuddy.utility.JavaModule; /** * This transformer applies {@link AllowAdvice} to every method registered with {@link * Feline#allowBlockingCallsInside(String, String)}. */ -class AllowancesTransformer implements AgentBuilder.Transformer { +class AllowancesTransformer implements AgentBuilder.Transformer, ElementMatcher { private final ConcurrentMap> allowances; @@ -52,8 +52,9 @@ void allow(final String className, final String methodName) { }); } - boolean containsClass(String name) { - return allowances.containsKey(name); + @Override + public boolean matches(final TypeDescription typeDescription) { + return allowances.containsKey(typeDescription.getName()); } @Override diff --git a/feline/src/main/java/com/spotify/feline/Feline.java b/feline/src/main/java/com/spotify/feline/Feline.java index 8f27e15..bd03b26 100644 --- a/feline/src/main/java/com/spotify/feline/Feline.java +++ b/feline/src/main/java/com/spotify/feline/Feline.java @@ -18,7 +18,6 @@ import java.io.IOException; import java.lang.instrument.Instrumentation; -import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -206,7 +205,7 @@ public static boolean removeThreadLocalInitialValueConsumer(final Runnable consu .asTerminalTransformation() // Instrument allowed/disallowed methods - .type(it -> allowancesTransformer.containsClass(it.getName())) + .type(allowancesTransformer) .transform(allowancesTransformer) .asTerminalTransformation() From a67ddb9314a3d56e8584bdd30e1f317077623a9b Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Fri, 5 Aug 2022 08:01:47 -0400 Subject: [PATCH 3/3] rename variable to avoid name shadowing --- .../java/com/spotify/feline/AllowancesTransformer.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java b/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java index d162c50..e60fabc 100644 --- a/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java +++ b/feline/src/main/java/com/spotify/feline/AllowancesTransformer.java @@ -43,12 +43,12 @@ class AllowancesTransformer implements AgentBuilder.Transformer, ElementMatcher< void allow(final String className, final String methodName) { allowances.compute( className, - (key, allowances) -> { - if (allowances == null) { - allowances = new HashSet<>(); + (key, set) -> { + if (set == null) { + set = new HashSet<>(); } - allowances.add(methodName); - return allowances; + set.add(methodName); + return set; }); }