muyun12 commented on a change in pull request #7003:
URL: https://github.com/apache/skywalking/pull/7003#discussion_r637993265
##########
File path:
apm-sniffer/bootstrap-plugins/jdk-threading-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdk/threading/ThreadingMethodInterceptor.java
##########
@@ -34,11 +34,10 @@
public void beforeMethod(final EnhancedInstance objInst, final Method
method, final Object[] allArguments,
final Class<?>[] argumentsTypes, final MethodInterceptResult result) {
- AbstractSpan span =
ContextManager.createLocalSpan(generateOperationName(objInst, method));
- span.setComponent(ComponentsDefine.JDK_THREADING);
-
final Object storedField = objInst.getSkyWalkingDynamicField();
if (storedField != null) {
+ AbstractSpan span =
ContextManager.createLocalSpan(generateOperationName(objInst, method));
+ span.setComponent(ComponentsDefine.JDK_THREADING);
Review comment:
Yes, `beforeMethod` is always creating local span，and `afterMethod` can
not clear `ContextManager`'s `CONTEXT` and `RUNTIME_CONTEXT`, that will cause a
memory leak.
Add `objInst.getSkyWalkingDynamicField()` check，the problem of memory leak
can be solved by keeping the logic in `beforeMethod`, `afterMethod` and
`ThreadingConstructorInterceptor.onConstruct`.
In the ThreadingConstructorInterceptor class:
```java
// in our sence ContextManager.isActive() will return false，cause
if (ContextManager.isActive()) {
objInst.setSkyWalkingDynamicField(ContextManager.capture());
}
```
In the ThreadingMethodInterceptor class:
```
@Override
public void beforeMethod(final EnhancedInstance objInst, final Method
method, final Object[] allArguments,
final Class<?>[] argumentsTypes, final MethodInterceptResult result)
{
// LocalSpan is always created here and referenced by the worker of
ThreadPool
AbstractSpan span =
ContextManager.createLocalSpan(generateOperationName(objInst, method));
span.setComponent(ComponentsDefine.JDK_THREADING);
final Object storedField = objInst.getSkyWalkingDynamicField();
if (storedField != null) {
final ContextSnapshot contextSnapshot = (ContextSnapshot)
storedField;
ContextManager.continued(contextSnapshot);
}
}
@Override
public Object afterMethod(final EnhancedInstance objInst, final Method
method, final Object[] allArguments,
final Class<?>[] argumentsTypes, final Object ret) {
// ContextManager.CONTEXT cannot be cleaned up here
final Object storedField = objInst.getSkyWalkingDynamicField();
if (storedField != null) {
ContextManager.stopSpan();
}
return ret;
}
`````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
