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

[LOGBACK-1688] Fix request log on Jetty 10.0.x #586

Open
wants to merge 1 commit into
base: branch_1.3.x
Choose a base branch
from

Conversation

zUniQueX
Copy link

@zUniQueX zUniQueX commented Sep 2, 2022

The compiled JAR for logback-access:1.3.0 doesn't fully work with Jetty 10.0.x.

When decompiling the JAR with javap, the following Methodref is used in the JettyModernServerAdapter#buildResponseHeaderMap() method:

#49 = Methodref #14.#50 // org/eclipse/jetty/server/Response.getHttpFields:()Lorg/eclipse/jetty/http/HttpFields;

#50 = NameAndType #51:#52 // getHttpFields:()Lorg/eclipse/jetty/http/HttpFields;
#51 = Utf8 getHttpFields
#52 = Utf8 ()Lorg/eclipse/jetty/http/HttpFields;

Since the return type was changed to HttpFields.Mutable in Jetty 10.0.x, the method isn't found and the following exception is thrown:

java.lang.NoSuchMethodError: 'org.eclipse.jetty.http.HttpFields org.eclipse.jetty.server.Response.getHttpFields()'

However, in both Jetty versions the return types implement Iterable<HttpField>, so we can resolve the correct method via reflection and cast the result to Iterable<HttpField>.

Additionally, I think it's a good idea to make the makeJettyServerAdapter method protected to give users the flexibility to disable the reflection usage, if this gets a performance issue.

CC @joakime Maybe you can take a look at this, if this is correct or if I'm missing something.

Signed-off-by: Steffen Nießing <[email protected]>
@zUniQueX zUniQueX force-pushed the fix-jetty-requestlog branch from fc767b7 to ee7d2da Compare September 2, 2022 11:56
@zUniQueX zUniQueX changed the title Fix request log on Jetty 10.0.x [LOGBACK-1688] Fix request log on Jetty 10.0.x Sep 28, 2022
@zUniQueX
Copy link
Author

Anything I can do to make it easier getting this merged?

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.

1 participant