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

[native-image] Adds -H:+ForeignAPISupport for all GraalVM 24.2+ #44092

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

Karm
Copy link
Member

@Karm Karm commented Oct 24, 2024

fixes #44001
enables a disabled test, discussed in #43997

Due to the AWT test being disabled it has been slipping under the radar that latest JDK, i.e. JDK 24+ uses JEP 454 API internally at places previously catered to by plain JNI. The fonts detection and fonts loading is such a place. We reflected this change in Mandrel test suite Karm/mandrel-integration-tests#277, but it is finding its way to Quarkus only now with this PR.

See GraalVM notes on the support of this API in Native-image: foreign-interface

I tested this PR with latest JDK 24 based Mandrel dev build Mandrel-24.2.0-dev59c335af5b8 (build 24-beta+20-ea)
and the current GA Mandrel-24.1.1.0-Final (build 23.0.1+11).

Question:

Do we enable the API unconditionally for JDK 24+ based native-image? Or do we detect AWT extension usage? Which other use cases might trigger this in the JDK code?

Explanation of the debug

...for posterity.

The error message is cryptic for information leakage reasons as discussed on openjdk/jdk#20169 (comment).

Caused by: java.io.IOException: Problem reading font data.
	at java.desktop@24-beta/java.awt.Font.createFont0(Font.java:1205)
	at java.desktop@24-beta/java.awt.Font.createFont(Font.java:1076)
	at io.quarkus.awt.it.Application.init(Application.java:63)
	at io.quarkus.awt.it.Application_Bean.doCreate(Unknown Source)
	... 26 more

So the root cause became apparent to me during a gdb session:

(gdb) p *(t->cause->detailMessage->value)
$1054 = {<byte []> = {<java.lang.Object> = {<_objhdr> = {
        hub = 0xe093b8 <io.netty.handler.codec.http2.DefaultHttp2FrameReader::readHeadersFrame(io.netty.channel.ChannelHandlerContext*, 
io.netty.buffer.ByteBuf*, io.netty.handler.codec.http2.Http2FrameListener*)+1096>}, <No data fields>}, len = 101, 
data = 0x7fffed638eb0 "Support for the Java Foreign Function and Memory API is not active: enable with -H:+ForeignAPISupport"}, <No data fields>}
(gdb) 

...and only then I recalled the old Karm/mandrel-integration-tests#277.

@Karm Karm requested review from gsmet and zakkak October 24, 2024 23:33
@Karm Karm self-assigned this Oct 24, 2024
Copy link

quarkus-bot bot commented Oct 24, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

Copy link

quarkus-bot bot commented Oct 24, 2024

/cc @galderz (awt)

This comment has been minimized.

/*
* Foreign Function and Memory API in Native Image, JDK's JEP 454
* This is needed for JDK 24+ internal native calls due to AWT.
* TODO: Do we enable it globally like so or do we only enable it when AWT extension is used?
Copy link
Member

Choose a reason for hiding this comment

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

I think I would rather be consistent rather than enabling obscure options only when a very specific extension is enabled.

So +1 for your current approach but I also would like @zakkak 's opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern is that this option is experimental and that the FFM API support is still WIP.

@Karm is this necessary because we are using internal JDK code that depends on the FFM API or because some library in the classpath detects that we are using JDK 24 and tries to use FFM API? In the first case I guess we can't do much and I would prefer we enable the flag only when necessary (at least for now). In the latter case I believe we should see how to make the said library avoid the use of the FFM API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* TODO: Do we enable it globally like so or do we only enable it when AWT extension is used?

@Karm
Copy link
Member Author

Karm commented Oct 29, 2024

Just a rebase to account for #44139

@Karm
Copy link
Member Author

Karm commented Oct 29, 2024

@zakkak If you have any specific suggestion as to what to explore, gimme a hint. I think we eventually need this when Brotli4j switches to FFI too...

This comment has been minimized.

@zakkak
Copy link
Contributor

zakkak commented Oct 29, 2024

@zakkak If you have any specific suggestion as to what to explore, gimme a hint. I think we eventually need this when Brotli4j switches to FFI too...

At the moment it's not clear to me what code is using the FFM API when using AWT. Is it java.awt itself or some 3rd party library?

@Karm
Copy link
Member Author

Karm commented Oct 30, 2024

@zakkak If you have any specific suggestion as to what to explore, gimme a hint. I think we eventually need this when Brotli4j switches to FFI too...

At the moment it's not clear to me what code is using the FFM API when using AWT. Is it java.awt itself or some 3rd party library?

It's JDK itself, e.g. in sun.java2d, right off the bat: https://bugs.openjdk.org/browse/JDK-8337237

i.e. this Quarkus PR is not to satisfy any specific third party lib like TwelveMonkeys or Bioformats etc.

@zakkak
Copy link
Contributor

zakkak commented Oct 30, 2024

Good, in that case I guess enabling it by default (when using GraalVM for JDK >= 24) makes sense.

We could have a check to only do it when the corresponding internal libraries are reached but that will quickly get outdated.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

Thank you Karm.

Please remove the TODO and it's good to go :)

/*
* Foreign Function and Memory API in Native Image, JDK's JEP 454
* This is needed for JDK 24+ internal native calls due to AWT.
* TODO: Do we enable it globally like so or do we only enable it when AWT extension is used?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* TODO: Do we enable it globally like so or do we only enable it when AWT extension is used?

@Karm
Copy link
Member Author

Karm commented Oct 30, 2024

@zakkak Can't merge it until you waive it I guess " Merging can be performed automatically once the requested changes are addressed. "

@zakkak zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 30, 2024
Copy link

quarkus-bot bot commented Oct 30, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 42d7a13.

✅ 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.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/opentelemetry/deployment

io.quarkus.opentelemetry.deployment.metrics.HttpServerMetricsTest.collectsHttpRouteFromEndAttributes - History

  • Assertion condition defined as a Lambda expression in io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter expected: <true> but was: <false> within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter expected: <true> but was: <false> within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter.assertCountPointsAtLeast(InMemoryMetricExporter.java:131)
	at io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter_ClientProxy.assertCountPointsAtLeast(Unknown Source)

@zakkak zakkak merged commit 45dbff7 into quarkusio:main Oct 30, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 30, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 30, 2024
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.

AWT native extension needs porting to JDK 24 ea
3 participants