-
Notifications
You must be signed in to change notification settings - Fork 7
Propagate request context across JDK executors #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| package dev.aikido.agent.wrappers.executor; | ||
|
|
||
| import dev.aikido.agent.wrappers.Wrapper; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.description.method.MethodDescription; | ||
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| import java.util.concurrent.AbstractExecutorService; | ||
| import java.util.concurrent.Callable; | ||
|
|
||
| import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf; | ||
| import static net.bytebuddy.matcher.ElementMatchers.named; | ||
| import static net.bytebuddy.matcher.ElementMatchers.takesArguments; | ||
|
|
||
| public class AbstractExecutorServiceWrapper implements Wrapper { | ||
| @Override | ||
| public String getName() { | ||
| return SubmitAdvice.class.getName(); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher getMatcher() { | ||
| return isMethod() | ||
| .and(named("submit")) | ||
| .and( | ||
| takesArguments(Runnable.class) | ||
| .or(takesArguments(Callable.class)) | ||
| .or(takesArguments(Runnable.class, Object.class)) | ||
| ); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher getTypeMatcher() { | ||
| return isSubTypeOf(AbstractExecutorService.class); | ||
| } | ||
|
|
||
| public static class SubmitAdvice { | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void before( | ||
| @Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task | ||
| ) { | ||
| if (task instanceof Runnable) { | ||
| task = ExecutorContextPropagation.wrap((Runnable) task); | ||
| } else if (task instanceof Callable) { | ||
| task = ExecutorContextPropagation.wrap((Callable) task); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| package dev.aikido.agent.wrappers.executor; | ||
|
|
||
| import dev.aikido.agent.wrappers.Wrapper; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.description.method.MethodDescription; | ||
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| import java.lang.reflect.Method; | ||
| import java.net.URL; | ||
| import java.net.URLClassLoader; | ||
| import java.util.concurrent.Callable; | ||
|
|
||
| import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||
| import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; | ||
| import static net.bytebuddy.matcher.ElementMatchers.named; | ||
| import static net.bytebuddy.matcher.ElementMatchers.takesArguments; | ||
|
|
||
| public class DelegatedExecutorServiceWrapper implements Wrapper { | ||
| @Override | ||
| public String getName() { | ||
| return DelegatedExecutorAdvice.class.getName(); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher getMatcher() { | ||
| return isMethod() | ||
| .and(named("execute").or(named("submit"))) | ||
| .and( | ||
| takesArguments(Runnable.class) | ||
| .or(takesArguments(Callable.class)) | ||
| .or(takesArguments(Runnable.class, Object.class)) | ||
| ); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher getTypeMatcher() { | ||
| return nameStartsWith("java.util.concurrent.Executors$"); | ||
| } | ||
|
|
||
| public static class DelegatedExecutorAdvice { | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void before( | ||
| @Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task | ||
| ) throws Exception { | ||
| if (task == null) { | ||
| return; | ||
| } | ||
|
|
||
| // This advice is applied to JDK classes loaded by the bootstrap classloader. | ||
| // Load agent_api reflectively because bootstrap classes cannot directly reference agent classes. | ||
| String jarFilePath = System.getProperty("AIK_agent_api_jar"); | ||
| if (jarFilePath == null || jarFilePath.isBlank()) { | ||
| return; | ||
| } | ||
|
|
||
| // The wrapper returned by wrap() resolves through the parent (system) classloader, | ||
| // so this per-call loader can be closed once wrapping is done. | ||
| URLClassLoader classLoader = new URLClassLoader(new URL[] { new URL(jarFilePath) }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Creating and closing a URLClassLoader for every delegated executor submission adds substantial repeated class-loading overhead on a high-frequency task-submission path. Details✨ AI Reasoning 🔧 How do I fix it? Reply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The explicit try/finally around classLoader.close() is unnecessarily verbose for a scoped resource. Use try-with-resources to express the same lifecycle more directly. Details✨ AI Reasoning 🔧 How do I fix it? Reply |
||
| try { | ||
| Class<?> contextPropagationClass = classLoader.loadClass( | ||
| "dev.aikido.agent_api.context.ContextPropagation" | ||
| ); | ||
|
|
||
| if (task instanceof Runnable) { | ||
| Method wrapRunnable = contextPropagationClass.getMethod("wrap", Runnable.class); | ||
| task = wrapRunnable.invoke(null, task); | ||
|
Comment on lines
+58
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium - Delegated and scheduled executor wrappers reopen Both bootstrap-safe executor wrappers now allocate a fresh Show fixCache the reflected More info - Reply on this comment to give feedback or ignore the issue. |
||
| } else if (task instanceof Callable) { | ||
| Method wrapCallable = contextPropagationClass.getMethod("wrap", Callable.class); | ||
| task = wrapCallable.invoke(null, task); | ||
| } | ||
| } finally { | ||
| classLoader.close(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| package dev.aikido.agent.wrappers.executor; | ||
|
|
||
| import java.lang.reflect.Method; | ||
| import java.net.URL; | ||
| import java.net.URLClassLoader; | ||
| import java.util.concurrent.Callable; | ||
|
|
||
| // Bridges the executor advice (woven into java.util.concurrent classes) to ContextPropagation in | ||
| // agent_api, which lives on a different classloader, and caches the reflected methods so the lookup | ||
| // happens once. A missing AIK_agent_api_jar is treated as "not ready yet" (early startup) and | ||
| // retried on the next call, rather than disabling propagation for the whole JVM; only a genuine | ||
| // load failure once the path is set disables it. | ||
| public final class ExecutorContextPropagation { | ||
| private static volatile Method wrapRunnableMethod; | ||
| private static volatile Method wrapCallableMethod; | ||
| private static volatile boolean disabled; | ||
|
|
||
| private ExecutorContextPropagation() {} | ||
|
|
||
| public static Runnable wrap(Runnable task) { | ||
| if (task == null) { | ||
| return task; | ||
| } | ||
| Method wrap = wrapRunnableMethod; | ||
| if (wrap == null) { | ||
| init(); | ||
| wrap = wrapRunnableMethod; | ||
| } | ||
| if (wrap == null) { | ||
| return task; | ||
| } | ||
| try { | ||
| return (Runnable) wrap.invoke(null, task); | ||
| } catch (Throwable ignored) { | ||
| return task; | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public static <T> Callable<T> wrap(Callable<T> task) { | ||
| if (task == null) { | ||
| return task; | ||
| } | ||
| Method wrap = wrapCallableMethod; | ||
| if (wrap == null) { | ||
| init(); | ||
| wrap = wrapCallableMethod; | ||
| } | ||
| if (wrap == null) { | ||
| return task; | ||
| } | ||
| try { | ||
| return (Callable<T>) wrap.invoke(null, task); | ||
| } catch (Throwable ignored) { | ||
| return task; | ||
| } | ||
| } | ||
|
|
||
| private static synchronized void init() { | ||
| if (disabled || wrapRunnableMethod != null) { | ||
| return; | ||
| } | ||
| String jarFilePath = System.getProperty("AIK_agent_api_jar"); | ||
| if (jarFilePath == null || jarFilePath.isBlank()) { | ||
| return; // not set yet during early startup - retry on a later call | ||
| } | ||
| try { | ||
| URLClassLoader classLoader = new URLClassLoader(new URL[] { new URL(jarFilePath) }); | ||
| Class<?> clazz = classLoader.loadClass("dev.aikido.agent_api.context.ContextPropagation"); | ||
| wrapCallableMethod = clazz.getMethod("wrap", Callable.class); | ||
| wrapRunnableMethod = clazz.getMethod("wrap", Runnable.class); | ||
| } catch (Throwable ignored) { | ||
| disabled = true; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| package dev.aikido.agent.wrappers.executor; | ||
|
|
||
| import dev.aikido.agent.wrappers.Wrapper; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.description.method.MethodDescription; | ||
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| import java.util.concurrent.Callable; | ||
| import java.util.concurrent.ForkJoinPool; | ||
|
|
||
| import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf; | ||
| import static net.bytebuddy.matcher.ElementMatchers.named; | ||
| import static net.bytebuddy.matcher.ElementMatchers.takesArguments; | ||
|
|
||
| public class ForkJoinPoolWrapper implements Wrapper { | ||
| @Override | ||
| public String getName() { | ||
| return ForkJoinAdvice.class.getName(); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher getMatcher() { | ||
| return isMethod() | ||
| .and(named("execute").or(named("submit"))) | ||
| .and( | ||
| takesArguments(Runnable.class) | ||
| .or(takesArguments(Callable.class)) | ||
| .or(takesArguments(Runnable.class, Object.class)) | ||
| ); | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher getTypeMatcher() { | ||
| return isSubTypeOf(ForkJoinPool.class); | ||
| } | ||
|
|
||
| public static class ForkJoinAdvice { | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void before( | ||
| @Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task | ||
| ) { | ||
| if (task instanceof Runnable) { | ||
| task = ExecutorContextPropagation.wrap((Runnable) task); | ||
| } else if (task instanceof Callable) { | ||
| task = ExecutorContextPropagation.wrap((Callable) task); | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| package dev.aikido.agent.wrappers.executor; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| import dev.aikido.agent.wrappers.Wrapper; | ||||||||||||||||||||||||||||||||||||||||||||||
| import net.bytebuddy.asm.Advice; | ||||||||||||||||||||||||||||||||||||||||||||||
| import net.bytebuddy.description.method.MethodDescription; | ||||||||||||||||||||||||||||||||||||||||||||||
| import net.bytebuddy.description.type.TypeDescription; | ||||||||||||||||||||||||||||||||||||||||||||||
| import net.bytebuddy.matcher.ElementMatcher; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| import java.lang.reflect.Method; | ||||||||||||||||||||||||||||||||||||||||||||||
| import java.net.URL; | ||||||||||||||||||||||||||||||||||||||||||||||
| import java.net.URLClassLoader; | ||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.Callable; | ||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.ScheduledThreadPoolExecutor; | ||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.TimeUnit; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC; | ||||||||||||||||||||||||||||||||||||||||||||||
| import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||||||||||||||||||||||||||||||||||||||||||||||
| import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf; | ||||||||||||||||||||||||||||||||||||||||||||||
| import static net.bytebuddy.matcher.ElementMatchers.named; | ||||||||||||||||||||||||||||||||||||||||||||||
| import static net.bytebuddy.matcher.ElementMatchers.takesArguments; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| public class ScheduledThreadPoolExecutorWrapper implements Wrapper { | ||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||
| public String getName() { | ||||||||||||||||||||||||||||||||||||||||||||||
| return ScheduleAdvice.class.getName(); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||
| public ElementMatcher getMatcher() { | ||||||||||||||||||||||||||||||||||||||||||||||
| return isMethod() | ||||||||||||||||||||||||||||||||||||||||||||||
| .and(named("schedule")) | ||||||||||||||||||||||||||||||||||||||||||||||
| .and( | ||||||||||||||||||||||||||||||||||||||||||||||
| takesArguments(Runnable.class, long.class, TimeUnit.class) | ||||||||||||||||||||||||||||||||||||||||||||||
| .or(takesArguments(Callable.class, long.class, TimeUnit.class)) | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium - Periodic scheduled executor methods are left uninstrumented
Show fix
Suggested change
More info - Reply on this comment to give feedback or ignore the issue. |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||
| public ElementMatcher getTypeMatcher() { | ||||||||||||||||||||||||||||||||||||||||||||||
| return isSubTypeOf(ScheduledThreadPoolExecutor.class); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| public static class ScheduleAdvice { | ||||||||||||||||||||||||||||||||||||||||||||||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||||||||||||||||||||||||||||||||||||||||||||||
| public static void before( | ||||||||||||||||||||||||||||||||||||||||||||||
| @Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task | ||||||||||||||||||||||||||||||||||||||||||||||
| ) throws Exception { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (task == null) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // This advice is applied to JDK classes loaded by the bootstrap classloader. | ||||||||||||||||||||||||||||||||||||||||||||||
| // Load agent_api reflectively because bootstrap classes cannot directly reference agent classes. | ||||||||||||||||||||||||||||||||||||||||||||||
| String jarFilePath = System.getProperty("AIK_agent_api_jar"); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (jarFilePath == null || jarFilePath.isBlank()) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // The wrapper returned by wrap() resolves through the parent (system) classloader, | ||||||||||||||||||||||||||||||||||||||||||||||
| // so this per-call loader can be closed once wrapping is done. | ||||||||||||||||||||||||||||||||||||||||||||||
| URLClassLoader classLoader = new URLClassLoader(new URL[] { new URL(jarFilePath) }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Creating and closing a URLClassLoader for every scheduled task adds substantial repeated class-loading overhead on a high-frequency scheduling path. Details✨ AI Reasoning 🔧 How do I fix it? Reply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The explicit try/finally around classLoader.close() is unnecessarily verbose for a scoped resource. Use try-with-resources to express the same lifecycle more directly. Details✨ AI Reasoning 🔧 How do I fix it? Reply |
||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| Class<?> contextPropagationClass = classLoader.loadClass( | ||||||||||||||||||||||||||||||||||||||||||||||
| "dev.aikido.agent_api.context.ContextPropagation" | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (task instanceof Runnable) { | ||||||||||||||||||||||||||||||||||||||||||||||
| Method wrapRunnable = contextPropagationClass.getMethod("wrap", Runnable.class); | ||||||||||||||||||||||||||||||||||||||||||||||
| task = wrapRunnable.invoke(null, task); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else if (task instanceof Callable) { | ||||||||||||||||||||||||||||||||||||||||||||||
| Method wrapCallable = contextPropagationClass.getMethod("wrap", Callable.class); | ||||||||||||||||||||||||||||||||||||||||||||||
| task = wrapCallable.invoke(null, task); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||||||||||||||||||||
| classLoader.close(); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium - Bootstrap executor advice calls an agent-only helper that JDK classes cannot resolve
The new
AbstractExecutorService,ThreadPoolExecutor, andForkJoinPooladvices inject direct calls toExecutorContextPropagation.wrap(...)intojava.util.concurrentclasses even though this JVM setup never injects agent helper classes into the bootstrap classloader. The project already handles bootstrap-loaded JDK wrappers via reflective loading for that reason, and these advices also suppress any linkage error, sosubmit/executeon core executors silently runs without propagated request context. As a result, async work scheduled onto common JDK executors loses the request metadata that Zen uses to attribute and enforce protections on downstream sinks.Show fix
Do not reference
dev.aikido.agent...helpers directly from advice woven into bootstrap-loaded JDK classes. Either move these executor wrappers to the same reflective bridge pattern already used for bootstrap JDK wrappers, or explicitly append the helper classes to the bootstrap classloader search before instrumentingjava.util.concurrentso the injected calls can actually resolve.More info - Reply on this comment to give feedback or ignore the issue.