Skip to content

Fix Conflicts apm-jdk-threadpool-plugin conflicts with apm-jdk-forkjoinpool-plugin #753

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

Merged
merged 1 commit into from
Apr 29, 2025

Conversation

GuoHaoZai
Copy link
Contributor

@GuoHaoZai GuoHaoZai commented Apr 24, 2025

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #13204.
  • Update the CHANGES log.

return argument instanceof EnhancedInstance && ((EnhancedInstance) argument).getSkyWalkingDynamicField() instanceof ContextSnapshot;
return argument instanceof EnhancedInstance
&& ((EnhancedInstance) argument).getSkyWalkingDynamicField() instanceof ContextSnapshot
&& !(argument instanceof ForkJoinTask);
Copy link
Member

@wu-sheng wu-sheng Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When argument is ForkJoinTask, it will be repeatedly enhanced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.
Because forkjoinpool plugin enhanced is not work in threadpool plugin.
Also threadpool plugin enhanced is not work in forkjoinpool plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember when the previous two conditions matched, did you verify that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when forkjoinpool plugin enabled, it is matched

image

org.apache.skywalking.apm.plugin.jdk.forkjoinpool.ForkJoinTaskConstructorInterceptor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this plugin doesn't consider this kind of compatibility AFAIK. But still, these codes exist. I want to understand why to make sure the new added conditions don't impact existing features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember when the previous two conditions matched, did you verify that?我不记得前两个条件何时匹配,你验证过吗?

this is verify code and result. It looks normal

import lombok.extern.slf4j.Slf4j;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

import java.util.concurrent.*;

@Slf4j
@RestController
@RequestMapping(value = "/test")
public class TestController {

    ExecutorService executorService = Executors.newFixedThreadPool(10);

    @PutMapping("/test1")
    public void test() {
        executorService.submit(() -> {
            log.info("submit");
        });
        executorService.submit(new MyForkJoinTask(() -> {
            log.info("继承ForkJoinTask");
        }));
        CompletableFuture.runAsync(() -> {
            log.info("CompletableFuture底层的AsyncRun继承ForkJoinTask");
        }, executorService);
        ForkJoinPool.commonPool().execute((Runnable) new MyForkJoinTask(() -> {
            log.info("Runnable: 继承ForkJoinTask");
        }));
        ForkJoinPool.commonPool().execute((ForkJoinTask<Void>) new MyForkJoinTask(() -> {
            log.info("ForkJoinTask: 继承ForkJoinTask");
        }));
    }

    static final class MyForkJoinTask extends ForkJoinTask<Void> implements Runnable {
        CompletableFuture<Void> dep;
        Runnable fn;

        MyForkJoinTask(Runnable fn) {
            this.dep = new CompletableFuture<Void>();
            ;
            this.fn = fn;
        }

        public final Void getRawResult() {
            return null;
        }

        public final void setRawResult(Void v) {
        }

        public final boolean exec() {
            run();
            return false;
        }

        public void run() {
            CompletableFuture<Void> d;
            Runnable f;
            if ((d = dep) != null && (f = fn) != null) {
                dep = null;
                fn = null;
                if (!d.isDone()) {
                    try {
                        f.run();
                        d.complete(null);
                    } catch (Throwable ex) {
                        d.complete(null);
                    }
                }
            }
        }
    }

}

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I don't mean your codes are verified. I want to know why the method #notToEnhance existed before.

@wu-sheng wu-sheng changed the title Fix Conflicts apm-jdk-threadpool-plugin conflicts with apm-jdk-forkjo… Fix Conflicts apm-jdk-threadpool-plugin conflicts with apm-jdk-forkjoinpool-plugin Apr 24, 2025
@wu-sheng wu-sheng added enhancement New feature or request plugin labels Apr 24, 2025
@wu-sheng
Copy link
Member

@xu1009 @Cool-Coding If you are still watching here, could you join this review?

@GuoHaoZai
Copy link
Contributor Author

@xu1009 @Cool-Coding If you are still watching here, could you join this review?

Have any conclusion?

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could merge this first.

@wu-sheng wu-sheng merged commit 49d1946 into apache:main Apr 29, 2025
195 checks passed
@wu-sheng wu-sheng added this to the 9.5.0 milestone Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants