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

Logging with Panache: add support for method references of Log methods #46479

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Feb 25, 2025

In case the Log API is used in a lambda expression, everything already works, because a lambda expression body is compiled into a separate method and the Log API is invoked using classic invokestatic instruction.

However, in case the Log API is used as a method reference (Log::info), it breaks, because the LoggingWithPanacheProcessor fails to transform that usage. This commit fixes that by looking for invokedynamic invocations of the LambdaMetafactory where the method handle belongs to Log. Such instructions are rewritten to delegate to the JBoss Logging instance, as usual.

@geoand geoand requested a review from dmlloyd February 25, 2025 12:23
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Nice!

This comment has been minimized.

Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

LGTM, but let's make sure the test failures are unrelated.

In case the `Log` API is used in a lambda expression, everything already works,
because a lambda expression body is compiled into a separate method and
the `Log` API is invoked using classic `invokestatic` instruction.

However, in case the `Log` API is used as a method reference (`Log::info`),
it breaks, because the `LoggingWithPanacheProcessor` fails to transform that
usage. This commit fixes that by looking for `invokedynamic` invocations
of the `LambdaMetafactory` where the method handle belongs to `Log`. Such
instructions are rewritten to delegate to the JBoss Logging instance, as usual.
@Ladicek Ladicek force-pushed the logging-method-references-fix branch from 5e4f1cc to 44b3ce4 Compare February 26, 2025 08:40
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 26, 2025

Added a couple of comments and streamlined the code: instead of looking for the Handle in a loop, we access it directly, because for the LambdaMetafactory, it always comes on the 2nd position (therefore bootstrapMethodArguments[1]).

Also using this opportunity to rerun the failed tests, because those don't seem related on the first sight. If they fail again, I'll investigate properly :-)

Copy link

quarkus-bot bot commented Feb 26, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 44b3ce4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@Ladicek Ladicek merged commit 878285c into quarkusio:main Feb 26, 2025
58 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Feb 26, 2025
@Ladicek Ladicek deleted the logging-method-references-fix branch February 26, 2025 16:12
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.

3 participants