Skip to content
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

Fixing OSGi resolution issues caused by jdk.internal.vm.annotation #1247

Merged
merged 5 commits into from
Aug 21, 2020

Conversation

reta
Copy link
Contributor

@reta reta commented Aug 19, 2020

Addressed #1243, checked with 5.12.5-SNAPSHOT builds (local), the issue is gone. Thanks to @dkulp and @adriancole .

@codefromthecrypt
Copy link
Member

@reta thanks! can you verify that this doesn't recurse? ex the jars that depend on brave don't end up with the same problem. you can search for bnd files here or in console https://github.com/openzipkin/brave/find/master

this was one reason I was wondering if this could be handled at the plugin level.. possibly we should raise a JIRA on it? wdyt

@reta
Copy link
Contributor Author

reta commented Aug 20, 2020

@adriancole yes, sure, so far brave seems to be the only module which needs this dependency. CXF uses

io.zipkin.zipkin2:zipkin
io.zipkin.reporter2:zipkin-reporter
io.zipkin.reporter2:zipkin-reporter-brave
io.zipkin.brave:brave
io.zipkin.brave:brave-instrumentation-http

I have checked also

io.zipkin.brave:brave-instrumentation-messaging
io.zipkin.brave:brave-instrumentation-rpc

Others require dedicated JARs but none of them failed because of jdk.internal.vm.annotation

@codefromthecrypt
Copy link
Member

super weird.. I wonder then what is causing the dep on jdk.internal.vm.annotation...

@reta
Copy link
Contributor Author

reta commented Aug 20, 2020

@adriancole I found it, it is @Hidden annotation which comes from lambda- generated code for zipkin2.reporter.okhttp3.OkHttpSender class, it is in zipkin-sender-okhttp3 module

@codefromthecrypt
Copy link
Member

@reta cool. we can un-lambda that. great detective work!

@codefromthecrypt
Copy link
Member

So we only had that one lambda in all of the src/main of zipkin-reporter. I used intellij's "structural search" to verify the one you said was it (outside tests and benchmarks).

Screen Shot 2020-08-21 at 8 08 30 AM
Screen Shot 2020-08-21 at 8 08 43 AM
Screen Shot 2020-08-21 at 8 09 08 AM

@codefromthecrypt
Copy link
Member

It is still strange, though... isn't it?.. There's no compile dep on okhttp from io.zipkin.brave:brave

@reta
Copy link
Contributor Author

reta commented Aug 21, 2020

@adriancole that's correct, so if you disassemble it using javap, you will see the jdk/internal/vm/annotation/Hidden in there (at the end of the snippet, this class seems to be generated by retrolambda), for example (this is taken from 2.15.0 version):

$ javap -v OkHttpSender\$\$Lambda\$1.class                                                         
Classfile OkHttpSender$$Lambda$1.class                                                
  Last modified May 17, 2020; size 709 bytes                                                                   
  MD5 checksum 10eb8be2fbfee9f83f6a2b3b5b0193dd                                                                
  Compiled from "OkHttpSender.java"                                                                            
final class zipkin2.reporter.okhttp3.OkHttpSender$$Lambda$1 implements java.util.concurrent.ThreadFactory      
  minor version: 0                                                                                             
  major version: 51                                                                                            
  flags: (0x1030) ACC_FINAL, ACC_SUPER, ACC_SYNTHETIC                                                          
  this_class: #2                          // zipkin2/reporter/okhttp3/OkHttpSender$$Lambda$1                   
  super_class: #4                         // java/lang/Object                                                  
  interfaces: 1, fields: 1, methods: 4, attributes: 1                                                          
Constant pool:                                                                                                 
   #1 = Utf8               zipkin2/reporter/okhttp3/OkHttpSender$$Lambda$1                                     
   #2 = Class              #1             // zipkin2/reporter/okhttp3/OkHttpSender$$Lambda$1                   
   #3 = Utf8               java/lang/Object                                                                    
   #4 = Class              #3             // java/lang/Object                                                  
   #5 = Utf8               java/util/concurrent/ThreadFactory                                                  
   #6 = Class              #5             // java/util/concurrent/ThreadFactory                                
   #7 = Utf8               OkHttpSender.java                                                                   
   #8 = Utf8               instance                                                                            
   #9 = Utf8               Lzipkin2/reporter/okhttp3/OkHttpSender$$Lambda$1;                                   
  #10 = Utf8               <init>                                                                              
  #11 = Utf8               ()V                                                                                 
  #12 = NameAndType        #10:#11        // "<init>":()V                                                      
  #13 = Methodref          #4.#12         // java/lang/Object."<init>":()V                                     
  #14 = Utf8               newThread                                                                           
  #15 = Utf8               (Ljava/lang/Runnable;)Ljava/lang/Thread;                                            
  #16 = Utf8               Ljdk/internal/vm/annotation/Hidden;                                                 

What is mysterious though ... I was able to reproduce it only once locally by building the 2.15.1-SNAPSHOT ... I am still looking why it happened with 2.15.0 but not (anymore) with 2.15.1-SNAPSHOT. Building out of release tag (release-2.15.0) does not generate same bytecode either ....

@reta
Copy link
Contributor Author

reta commented Aug 21, 2020

It is still strange, though... isn't it?.. There's no compile dep on okhttp from io.zipkin.brave:brave

Not exactly but in test scope only:

$ mvn dependency:tree -pl :brave | grep okhttp3
[INFO] +- com.squareup.okhttp3:mockwebserver:jar:4.7.2:test
[INFO] |  \- com.squareup.okhttp3:okhttp:jar:4.7.2:test
[INFO] +- io.zipkin.reporter2:zipkin-sender-okhttp3:jar:2.15.0:test

(sorry, you are correct, there is no compile time dependency)

@codefromthecrypt
Copy link
Member

opened luontola/retrolambda#160 actually I found the culprit... we also had one here in BaggagePropagation

@codefromthecrypt
Copy link
Member

@reta I added a commit, that after you build openzipkin/zipkin-reporter-java#179 can verify if all clear?

@reta
Copy link
Contributor Author

reta commented Aug 21, 2020

awesome, thanks a lot @adriancole , I think we can close this pull request

@codefromthecrypt
Copy link
Member

nah can't close this as there's one here :D

brave/bnd.bnd Outdated Show resolved Hide resolved
@reta
Copy link
Contributor Author

reta commented Aug 21, 2020

@adriancole the MANIFEST.MF (for brave and zipkin-sender-okhttp3) are good, CXF build passes against the snapshots with no issues, thanks!

@@ -129,7 +129,7 @@
<maven-failsafe-plugin.version>3.0.0-M4</maven-failsafe-plugin.version>
<maven-invoker-plugin.version>3.2.1</maven-invoker-plugin.version>
<maven-enforcer-plugin.version>3.0.0-M3</maven-enforcer-plugin.version>
<maven-bundle-plugin.version>4.2.1</maven-bundle-plugin.version>
<maven-bundle-plugin.version>5.1.1</maven-bundle-plugin.version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@codefromthecrypt codefromthecrypt merged commit 0cc111d into openzipkin:master Aug 21, 2020
@codefromthecrypt
Copy link
Member

thanks.. I'll go ahead and ship this as don't want people pinned.

@codefromthecrypt
Copy link
Member

Mind having a look? #1252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants