Skip to content

Conversation

@jaikiran
Copy link
Member

Fixes #9047

Right now, access logging isn't working in native mode as reported in the issue. This is because the ExchangeAttributeBuilder(s) which are responsible for handling the access log format specifier are ServiceLoader based interfaces and as such need to be explicitly registered in native image for them to be available.

The commit here explicitly makes them available in native mode and also includes a test to reproduce the issue and verify the fix.

if (serviceInterfaceClassName == null || serviceInterfaceClassName.trim().isEmpty()) {
throw new IllegalArgumentException("service interface name cannot be null or blank");
}
final ClassLoader classLoaderToUse = cl == null ? Thread.currentThread().getContextClassLoader() : cl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

An issue with this approach is that if there are additional service implementations on the deployment class path that are not on the runtime class path this will cause failures. Not sure how likely this in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I hadn't thought of this scenario. I think I might have a slightly different way to achieve this same functionality without having to run into the issue that you note here. I'll try it out and update the PR shortly.

Copy link
Collaborator

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

LGTM, I don't think the ClassLoader issue will actually be a problem in practice.

…ttributeBuilder service providers for access logging to work in native image
@jaikiran
Copy link
Member Author

Hello @stuartwdouglas, given that the goal of this change was to register the "core" ExchangeAttributeBuilder(s) which are part of vertx-http, I've updated this PR to very selectively find that service descriptor file and load only those providers. This should avoid the classloader problem that you rightly raised previously.

@stuartwdouglas
Copy link
Collaborator

Thanks Jaikiran

@geoand geoand merged commit 8219a43 into quarkusio:master May 20, 2020
@geoand geoand added this to the 1.5.0.CR1 milestone May 20, 2020
@jaikiran jaikiran deleted the qk-9047 branch May 20, 2020 07:20
@rousseau-christopher
Copy link

thx guys for working on this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logging access log with native dont work.

4 participants