You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "2han9wen71an (via GitHub)" <gi...@apache.org> on 2023/02/03 06:24:35 UTC

[GitHub] [skywalking-java] 2han9wen71an opened a new pull request, #448: Fix osgi environment not working

2han9wen71an opened a new pull request, #448:
URL: https://github.com/apache/skywalking-java/pull/448

   - [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
   
   Reason:
   ---
   In osgi environment, each bundle has its own unique class loader, while `skywalking agent` runs in the system `AppClassLoader`, which will cause Skywalking plugin injection to fail
   
   
   Solution:
   ---
   Check the system and find that the Skywalking plugin uses `AgentClassLoader` to override `loadClass` and `findResources` for loading and processing, which is not a problem in a normal java environment, because the application package and Agent are in the `AppClassLoader`.
   We need to adjust the `AgentClassLoader` to adjust the class loader of the bundle to the `parent` of the AgentClassLoader, at this time the AgentClassLoader can access both the content in the bundle and the skywalking plugin, after consulting the information, we found that the JDK provides `URLClassLoader` just meet the conditions, so I carried out this refactoring
   Because of writing test cases to get the plug-in class need to write dead plug-in path, but plug-in we are not 100% enabled, so this PR I did not provide test cases, but in my local `Polarion` environment through the implementation
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an commented on a diff in pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096489957


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -169,31 +135,15 @@ private Class<?> processLoadedClass(Class<?> loadedClass) {
         return loadedClass;
     }
 
-    private List<Jar> getAllJars() {
-        if (allJars == null) {
-            jarScanLock.lock();
-            try {
-                if (allJars == null) {
-                    allJars = doGetJars();
-                }
-            } finally {
-                jarScanLock.unlock();
-            }
-        }
-
-        return allJars;
-    }
-
-    private LinkedList<Jar> doGetJars() {
-        LinkedList<Jar> jars = new LinkedList<>();
+    private static LinkedList<URL> doGetJars(List<File> classpath) {

Review Comment:
   Because it needs to be called in a static method, and only need to initialize the plugin list once



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1415243452

   Could we build a simple osgi test? Otherwise, if people report bugs, we don't have things to share about which cases we tested.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an commented on a diff in pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096491318


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -72,89 +100,27 @@ public static AgentClassLoader getDefault() {
      *
      * @throws AgentPackageNotFoundException if agent package is not found.
      */
-    public static void initDefaultLoader() throws AgentPackageNotFoundException {
+    public static void initDefaultLoader() {
         if (DEFAULT_LOADER == null) {
             synchronized (AgentClassLoader.class) {
                 if (DEFAULT_LOADER == null) {
-                    DEFAULT_LOADER = new AgentClassLoader(PluginBootstrap.class.getClassLoader());
+                    DEFAULT_LOADER = new AgentClassLoader(PLUGIN_JAR_URLS, PluginBootstrap.class.getClassLoader());
                 }
             }
         }
     }
 
-    public AgentClassLoader(ClassLoader parent) throws AgentPackageNotFoundException {
-        super(parent);

Review Comment:
   Not so, because you override the `findClass` method, which only reads the plugin directory and does not call parent `findClass`



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an commented on pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1416659096

   > BTW, could you draw some graphs to help me understanding the new VS old class loader workflow? I don't have enough background knowledge about osgi
   Figure `classloaderA` for bundle custom class loader, it can be adjusted by the parameter `org.osgi.framework.bundle.parent` to load the parent class as `boot` or `app`, but the class in the parent class loader is not able to get the class loaded by the child class loader, will lead to `Jedis` can not get
   ![A1AE4A5BF7EBF29F70372F766682603A](https://user-images.githubusercontent.com/22495231/216748699-6910a18e-2a23-409a-b335-410a82d64a24.png)
   So I made an adjustment and set the bundle class loader to be the `parent` of the `AgentClassLoader`, which solves the problem
   ![4366CC8225A5D7FBAA3EC018DDD59DB4](https://user-images.githubusercontent.com/22495231/216748705-395efa24-1deb-4928-b3d9-a6f2fa1ec3d6.png)
   
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1416661478

   Could you update graph in English? We need this available for global users.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096514472


##########
docs/menu.yml:
##########
@@ -96,6 +96,8 @@ catalog:
     catalog:
       - name: "Why is java.ext.dirs not supported?"
         path: "/en/faq/ext-dirs"
+      - name: "Why Skywalking doesn't work in `OSGI` environment?"

Review Comment:
   ```suggestion
         - name: "How to make SkyWalking agent works in `OSGI` environment?"
   ```



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1416695102

   I did an off-list discussion with @2han9wen71an 
   The issue seems to be able to be fixed through two parameters of OSGI.
   ```
   -Dorg.osgi.framework.bundle.parent=app
   -Dorg.osgi.framework.bootdelegation=*
   ```
   
   I would recommend writing a new FAQ to guide OSGI users.
   - A new doc could be added here, https://github.com/apache/skywalking-java/tree/main/docs/en/faq
   - The menu should be updated as well, https://github.com/apache/skywalking-java/blob/8dde931b728e82339df1f7eff5b808dd88a66024/docs/menu.yml#L95-L98
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096514530


##########
docs/en/faq/osgi.md:
##########
@@ -0,0 +1,15 @@
+## Why `Skywalking` doesn't work in `OSGI` environment?

Review Comment:
   ```suggestion
   ## How to make SkyWalking agent works in `OSGI` environment?
   ```



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096484275


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -72,89 +100,27 @@ public static AgentClassLoader getDefault() {
      *
      * @throws AgentPackageNotFoundException if agent package is not found.
      */
-    public static void initDefaultLoader() throws AgentPackageNotFoundException {
+    public static void initDefaultLoader() {
         if (DEFAULT_LOADER == null) {
             synchronized (AgentClassLoader.class) {
                 if (DEFAULT_LOADER == null) {
-                    DEFAULT_LOADER = new AgentClassLoader(PluginBootstrap.class.getClassLoader());
+                    DEFAULT_LOADER = new AgentClassLoader(PLUGIN_JAR_URLS, PluginBootstrap.class.getClassLoader());
                 }
             }
         }
     }
 
-    public AgentClassLoader(ClassLoader parent) throws AgentPackageNotFoundException {
-        super(parent);

Review Comment:
   This super should already make the class loader reading target class(like Jedis class).



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1095773120


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -44,25 +40,57 @@
 /**
  * The <code>AgentClassLoader</code> represents a classloader, which is in charge of finding plugins and interceptors.
  */
-public class AgentClassLoader extends ClassLoader {
+public class AgentClassLoader extends URLClassLoader {
+    private static final ILog LOGGER;
+    /**
+     * Storage plug-in jar package url address
+     */
+    private static URL[] ARRAY_PLUGINS_URL;

Review Comment:
   Could explain what is `ARRAY_PLUGINS_URL`? The name seems strange.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1415666680

   There are many tests, please ref this doc to add a new one?
   
   https://skywalking.apache.org/docs/skywalking-java/next/en/setup/service-agent/java-agent/plugin-test/


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096496051


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -72,89 +100,27 @@ public static AgentClassLoader getDefault() {
      *
      * @throws AgentPackageNotFoundException if agent package is not found.
      */
-    public static void initDefaultLoader() throws AgentPackageNotFoundException {
+    public static void initDefaultLoader() {
         if (DEFAULT_LOADER == null) {
             synchronized (AgentClassLoader.class) {
                 if (DEFAULT_LOADER == null) {
-                    DEFAULT_LOADER = new AgentClassLoader(PluginBootstrap.class.getClassLoader());
+                    DEFAULT_LOADER = new AgentClassLoader(PLUGIN_JAR_URLS, PluginBootstrap.class.getClassLoader());
                 }
             }
         }
     }
 
-    public AgentClassLoader(ClassLoader parent) throws AgentPackageNotFoundException {
-        super(parent);

Review Comment:
   `ClassLoader#loadClass` should cover similar logic about looking for parent class loader too.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1416716130

   Replaced by https://github.com/apache/skywalking-java/pull/450


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1095774108


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -44,25 +40,57 @@
 /**
  * The <code>AgentClassLoader</code> represents a classloader, which is in charge of finding plugins and interceptors.
  */
-public class AgentClassLoader extends ClassLoader {
+public class AgentClassLoader extends URLClassLoader {
+    private static final ILog LOGGER;
+    /**
+     * Storage plug-in jar package url address
+     */
+    private static URL[] ARRAY_PLUGINS_URL;

Review Comment:
   It looks like `PLUGIN_JAR_URLS`?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096514446


##########
docs/en/faq/osgi.md:
##########
@@ -0,0 +1,15 @@
+## Why `Skywalking` doesn't work in `OSGI` environment?
+In a normal Java application, our application code is usually loaded directly with `AppClassLoader`, when it is started with `-javaagent:<agent_root>\skywalking-agent.jar`,
+The code in `agent` is also loaded by `AppClassLoader`, while `Skywalking Plugins` is loaded by `AgentClassLoader`.
+The parent class loader of `AgentClassLoader` is the class loader of the application code, which is `AppClassLoader`, so the code related to `Skywalking` is running in `AppClassLoader`.
+
+But `OSGI` implements its own set of [modularity](https://www.osgi.org/resources/modularity/), which means that each `Bundle` has its own unique class loader for isolating different versions of classes.
+In `OSGI` start `agent` for class conversion interception, at this time the class loader of the application code is the unique class loader of the bundle, when the class related to `Skywalking` is needed
+will throw `java.lang.ClassNotFoundException` exception. Resulting in a startup exception
+
+### How to resolve this issue?
+1. we need to adjust the father of the class loader in `OSGI` to `AppClassLoader`, with the specific parameter `org.osgi.framework.bundle.parent=app`, because the default parent class of each bundle is boot

Review Comment:
   ```suggestion
   1. we need to adjust the parent of the classloader in `OSGI` to `AppClassLoader`, through the specific parameter `org.osgi.framework.bundle.parent=app`. Otherwise, the default parent classloader of each bundle is boot classloader.
   ```



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1416664089

   > > BTW, could you draw some graphs to help me understanding the new VS old class loader workflow? I don't have enough background knowledge about osgi
   > 
   > 
   > Figure `classloaderA` for bundle custom class loader, it can be adjusted by the parameter `org.osgi.framework.bundle.parent` to load the parent class as `boot` or `app`, but the class in the parent class loader is not able to get the class loaded by the child class loader, will lead to `Jedis` can not get
   > 
   > ![A1AE4A5BF7EBF29F70372F766682603A](https://user-images.githubusercontent.com/22495231/216748699-6910a18e-2a23-409a-b335-410a82d64a24.png)
   > 
   > So I made an adjustment and set the bundle class loader to be the `parent` of the `AgentClassLoader`, which solves the problem
   > 
   > ![4366CC8225A5D7FBAA3EC018DDD59DB4](https://user-images.githubusercontent.com/22495231/216748705-395efa24-1deb-4928-b3d9-a6f2fa1ec3d6.png)
   > 
   
   
   About this, the agent classloader should already have target class loader as parent to help to load classes. 
   What is missing?


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096483746


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/InterceptorInstanceLoader.java:
##########
@@ -62,7 +62,7 @@ public static <T> T load(String className,
             try {
                 pluginLoader = EXTEND_PLUGIN_CLASSLOADERS.get(targetClassLoader);
                 if (pluginLoader == null) {
-                    pluginLoader = new AgentClassLoader(targetClassLoader);
+                    pluginLoader = AgentClassLoader.getClassLoader(targetClassLoader);

Review Comment:
   So, you use the cached plugin classloader? I am not following.
   
   EXTEND_PLUGIN_CLASSLOADERS is already a cache, why AgentClassLoader builds a cache again?



##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -169,31 +135,15 @@ private Class<?> processLoadedClass(Class<?> loadedClass) {
         return loadedClass;
     }
 
-    private List<Jar> getAllJars() {
-        if (allJars == null) {
-            jarScanLock.lock();
-            try {
-                if (allJars == null) {
-                    allJars = doGetJars();
-                }
-            } finally {
-                jarScanLock.unlock();
-            }
-        }
-
-        return allJars;
-    }
-
-    private LinkedList<Jar> doGetJars() {
-        LinkedList<Jar> jars = new LinkedList<>();
+    private static LinkedList<URL> doGetJars(List<File> classpath) {

Review Comment:
   Why is this static?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1095776168


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -44,25 +40,57 @@
 /**
  * The <code>AgentClassLoader</code> represents a classloader, which is in charge of finding plugins and interceptors.
  */
-public class AgentClassLoader extends ClassLoader {
+public class AgentClassLoader extends URLClassLoader {
+    private static final ILog LOGGER;
+    /**
+     * Storage plug-in jar package url address
+     */
+    private static URL[] ARRAY_PLUGINS_URL;

Review Comment:
   By reading this, I feel your PR should be separated into two parts.
   1. Use URLClassLoader as the parent of `AgentClassLoader`. I know this could be done, but don't do this as didn't see the necessity
   2. Really add OSGI relative codes.
   
   The reason I proposed this is, I want to the change log clear and review easier.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096496051


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -72,89 +100,27 @@ public static AgentClassLoader getDefault() {
      *
      * @throws AgentPackageNotFoundException if agent package is not found.
      */
-    public static void initDefaultLoader() throws AgentPackageNotFoundException {
+    public static void initDefaultLoader() {
         if (DEFAULT_LOADER == null) {
             synchronized (AgentClassLoader.class) {
                 if (DEFAULT_LOADER == null) {
-                    DEFAULT_LOADER = new AgentClassLoader(PluginBootstrap.class.getClassLoader());
+                    DEFAULT_LOADER = new AgentClassLoader(PLUGIN_JAR_URLS, PluginBootstrap.class.getClassLoader());
                 }
             }
         }
     }
 
-    public AgentClassLoader(ClassLoader parent) throws AgentPackageNotFoundException {
-        super(parent);

Review Comment:
   `ClassLoader#loadClass` should have covered similar logic about looking for parent class loader too.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an closed pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an closed pull request #448: Fix osgi environment not working
URL: https://github.com/apache/skywalking-java/pull/448


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1416715589

   Oops, if you want to use this PR, you need to revert all code-relative changes.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an commented on a diff in pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096482318


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/AgentClassLoader.java:
##########
@@ -44,25 +40,57 @@
 /**
  * The <code>AgentClassLoader</code> represents a classloader, which is in charge of finding plugins and interceptors.
  */
-public class AgentClassLoader extends ClassLoader {
+public class AgentClassLoader extends URLClassLoader {
+    private static final ILog LOGGER;
+    /**
+     * Storage plug-in jar package url address
+     */
+    private static URL[] ARRAY_PLUGINS_URL;

Review Comment:
   `ARRAY_PLUGINS_URL` is because the plugin url is an array type, it is not as clear as `PLUGIN_JAR_URLS`, I will adjust it here.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an commented on a diff in pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096490680


##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/loader/InterceptorInstanceLoader.java:
##########
@@ -62,7 +62,7 @@ public static <T> T load(String className,
             try {
                 pluginLoader = EXTEND_PLUGIN_CLASSLOADERS.get(targetClassLoader);
                 if (pluginLoader == null) {
-                    pluginLoader = new AgentClassLoader(targetClassLoader);
+                    pluginLoader = AgentClassLoader.getClassLoader(targetClassLoader);

Review Comment:
   `EXTEND_PLUGIN_CLASSLOADERS` is a storage place to get the `AgentClassLoader`, and the `AgentClassLoader.getClassLoader` method is to get all the classes in the bundle and the class loader of the skywalking plugin, which can be used for other places later



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#discussion_r1096514590


##########
docs/en/faq/osgi.md:
##########
@@ -0,0 +1,15 @@
+## Why `Skywalking` doesn't work in `OSGI` environment?
+In a normal Java application, our application code is usually loaded directly with `AppClassLoader`, when it is started with `-javaagent:<agent_root>\skywalking-agent.jar`,
+The code in `agent` is also loaded by `AppClassLoader`, while `Skywalking Plugins` is loaded by `AgentClassLoader`.
+The parent class loader of `AgentClassLoader` is the class loader of the application code, which is `AppClassLoader`, so the code related to `Skywalking` is running in `AppClassLoader`.
+
+But `OSGI` implements its own set of [modularity](https://www.osgi.org/resources/modularity/), which means that each `Bundle` has its own unique class loader for isolating different versions of classes.
+In `OSGI` start `agent` for class conversion interception, at this time the class loader of the application code is the unique class loader of the bundle, when the class related to `Skywalking` is needed
+will throw `java.lang.ClassNotFoundException` exception. Resulting in a startup exception

Review Comment:
   Could you add the typical exception stack here?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng closed pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #448: Fix osgi environment not working
URL: https://github.com/apache/skywalking-java/pull/448


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] 2han9wen71an commented on pull request #448: Fix osgi environment not working

Posted by "2han9wen71an (via GitHub)" <gi...@apache.org>.
2han9wen71an commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1415603795

   > Could we build a simple osgi test? Otherwise, if people report bugs, we don't have things to share about which cases we tested.
   
   I just created a simple test
   1. download the felix framework https://felix.apache.org/documentation/downloads.html
   2. compile using the project https://github.com/StasKolodyuk/osgi-spring-boot-demo
   3. put the compiled jar into the bundle directory in felix
   4. start `java -jar <felix_path>/bin/felix.jar`
   Attach my vm parameters
   -javaagent: "D:\skywalking-agent\skywalking-agent.jar"
   -Dskywalking.agent.service_name=sample
   -Dskywalking.collector.backend_service=xxxx:11800
   -Dorg.osgi.framework.bundle.parent=app
   -Dorg.osgi.framework.bootdelegation=*
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-java] wu-sheng commented on pull request #448: Fix osgi environment not working

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #448:
URL: https://github.com/apache/skywalking-java/pull/448#issuecomment-1415836354

   BTW, could you draw some graphs to help me understanding the new VS old class loader workflow? 
   I don't have enough background knowledge about osgi


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org