[GitHub] [skywalking] TomMD commented on a change in pull request #6103: new feature add witness method

[GitBox <gi...@apache.org>]

TomMD commented on a change in pull request #6103:
URL: https://github.com/apache/skywalking/pull/6103#discussion_r550234070



##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/WitnessClassFinder.java
##########
@@ -36,6 +36,17 @@
      * @return true, if the given witnessClass exists, through the given classLoader.
      */
     public boolean exist(String witnessClass, ClassLoader classLoader) {
+        return getResolution(witnessClass, classLoader)

Review comment:
       @wu-sheng I haven't looked at the actual trace produced by the tool but here are my thoughts - please let me know if they seem off and I can ask the tool for its explanation.
   
   The message here is due to the same core issue as you see in the `getResolution` comment below.  When calling `getResolution` then without synchronization we execute:
   
   ```
    TypePool typePool = poolMap.get(mappingKey);
   ```
   
   And that can race with a write to `poolMap` occurring in another thread.  There might not be a `mappingKey` inside of `poolMap` because `poolMap` could be modified prior to calling `get`.  If you'd like to be sure the element exists then you'd need the synchronize block to encompass both the `get` and the `containsKey` call.
   
   We're planning on making traces accessible from the comments with a commands to the bot - this would illuminate the supposed bugs and help developers decide if the trace is feasible and worth changing.  We welcome any thoughts you have about bug explanation and elaboration.




----------------------------------------------------------------
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:
users@infra.apache.org