Limit external configuration sources parsing in native-mode #42140
Conversation
|
@radcortez please take a look. I opened it as draft since there are way more reflective access being caught by The ones I am trying to understand now are ones generated because of the sources automatically added to the RunTimeConfigCustomizer. Do you think we could/should skip some of those when targeting native-mode, or are they all necessary? Since I have your attention I would also like to ask why you are not generating Thanks |
The idea is to discover all the services during the build and generate a config customizer that programmatically registers all the discovered services. I think we cannot skip any of the things we discover. Are you seeing issues with this?
We probably don't need the reflection registration anymore. We can try to drop it and check :) |
| @@ -391,6 +392,17 @@ public void setupConfigOverride( | |||
|
|
|||
| @BuildStep | |||
| public void watchConfigFiles(BuildProducer<HotDeploymentWatchedFileBuildItem> watchedFiles) { | |||
There was a problem hiding this comment.
The watched files include external files like .env and {user.dir}/config/application{-profile}.properties. Should these be registered as a resource with the native image?
There was a problem hiding this comment.
Anything accessed through java.lang.ClassLoader.getResources (invoked by https://github.com/smallrye/smallrye-common/blob/c29bb1c00f484f5fa86f183e75c37af462ac6546/classloader/src/main/java/io/smallrye/common/classloader/ClassPathUtils.java#L84) needs to be registered (at least in theory).
There was a problem hiding this comment.
External files should be calling processAsPath https://github.com/smallrye/smallrye-common/blob/c29bb1c00f484f5fa86f183e75c37af462ac6546/classloader/src/main/java/io/smallrye/common/classloader/ClassPathUtils.java#L132-L160 directly, without using the classloader:
For {user.dir}/config/application.properties:
https://github.com/smallrye/smallrye-config/blob/e8d87ce1cfbf1e6f259a798c85498267dd12b32b/implementation/src/main/java/io/smallrye/config/AbstractLocationConfigSourceLoader.java#L118-L135
At least until now, we have never had issues loading these files and the native executable.
There was a problem hiding this comment.
Yes, these "issues" are popping up when using -H:+ThrowMissingRegistrationErrors (see #41995) which aims to help developers detect unregistered accesses to elements. What I suspect is that there are many places where Quarkus tries to access unregistered elements and just fails without a warning, but since there are other places or ways to get the same data back it works as expected.
So in many of these cases it might be OK to not register the elements, but at the same time the best practice would be to ensure hat Quarkus also doesn't try to access them at run-time (e.g. through substitutions or changing configuration).
There was a problem hiding this comment.
@radcortez note that when the uri scheme is not defined smallrye tries both tryFileSystem and tryClassPath in https://github.com/smallrye/smallrye-config/blob/e8d87ce1cfbf1e6f259a798c85498267dd12b32b/implementation/src/main/java/io/smallrye/config/AbstractLocationConfigSourceLoader.java#L103-L104 with the latter accessing (or trying to) the config files through java.lang.ClassLoader.getResources.
If we don't want the external files to even be tried I think we should alter the value of smallrye.config.locations. WDYT?
| List<String> configWatchedFiles = new ArrayList<>(); | ||
| String userDir = System.getProperty("user.dir"); | ||
|
|
||
| // Main files | ||
| configWatchedFiles.add("META-INF/microprofile-config.yaml"); | ||
| configWatchedFiles.add("META-INF/microprofile-config.yml"); | ||
| configWatchedFiles.add("application.yaml"); | ||
| configWatchedFiles.add("application.yml"); | ||
| configWatchedFiles.add(Paths.get(userDir, "config", "application.yaml").toAbsolutePath().toString()); |
There was a problem hiding this comment.
Check previous comments about external files.
...ntime/src/main/java/io/quarkus/runtime/configuration/FileSystemOnlySourcesConfigBuilder.java
Outdated
Show resolved
Hide resolved
...ntime/src/main/java/io/quarkus/runtime/configuration/FileSystemOnlySourcesConfigBuilder.java
Outdated
Show resolved
Hide resolved
Not necessarily, it kind of complicates the resource registration process so I am just trying to see if there is any opportunity for trimming down :) |
It looks like you are right. |
zakkak
force-pushed
the
2024-07-24-smallryeconfigsources
branch
from
fc678ed to
628ee63
Compare
Hum, the goal was to avoid as many registrations as possible. Since this has been an ongoing process, there are probably some code leftovers that are not required anymore (the reflection registration of services, for instance). Let me know what you are seeing and we try to hax a few things. |
|
I think we can drop registrations of for everything except |
If you run: you will see a bunch of exceptions being thrown, indicating that the resulting native-image is trying to access elements that have not been registered at build time. Unfortunately the A couple of indicative examples: |
Great. Thanks. Ok, let's try to trim these down. Did you run with these PR changes? It shouldn't be looking for |
Yes, it removes some of these warnings but not all. |
Yes, some of the warnings related to the lookups to the config files should be gone now. Looking into other warnings, I see many related to the Validator. The issue here is that we only register for reflection the config classes that have validation annotations. The config code still calls There is no quick fix for this. I guess the best option is to override the Validator and pass in a list of config classes that require validation to avoid that call. |
zakkak
force-pushed
the
2024-07-24-smallryeconfigsources
branch
from
628ee63 to
c005383
Compare
|
🎊 PR Preview 8bfec22 has been successfully built and deployed to https://quarkus-pr-main-42140-preview.surge.sh/version/main/guides/
|
`SmallryeConfigBuilder` tries to access properties files from various sources, despite them not being available at run-time nor supported in native-mode, see quarkusio#41994 This patch also avoids getting a `MissingRegistrationError` when using `-H:+ThrowMissingRegistrationErrors` or `--exact-reachability-metadata`. Related to quarkusio#41994 and quarkusio#41995
zakkak
force-pushed
the
2024-07-24-smallryeconfigsources
branch
from
c005383 to
0f37d9f
Compare
|
@radcortez please take another look. |
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
Ok, what is the goal now? :) Is it to include the configuration files in the native image because of #44652 (comment)? Isn't this going to hurt the native image startup time? |
Status for workflow
|
The goal is to not have code accessing unregistered resources. The trivial approach is to just register everything that's being accessed, but as you say this might impact performance. So ideally we would like to eliminate unnecessary accesses (and as a result avoid the registration of the corresponding resources). The first step towards that was to use a different The next step I guess is what you describe in #44652 (review)
but that's something I can't handle myself :) |
Yes, that is why I was asking. Because for native image we don't use the resources, so just disabling some of the discovery pieces would be enough. That problem would still exist, but it is no different from what we have today. Everything that comes from the classpath is recorded directly in bytecode. It is bit tricky to follow it in the code. Here, we collect the values: We use a customized instance of And we write the values here: Let me have a look and try to come up with something that can avoid these lookups, but without requiring the resouces. |
|
Here is an alternate proposal: #44910 Let's see if nothing breaks. |
I gave #44910 a try and I still see the following accesses: |
|
Which example are you using? One general problem is how do we handle libraries that may look for stuff but not necessarily require a Quarkus extension? For instance, I notice there that the app uses It does seem silly to require an extension to remove / register resources for every dependency, and impractical to have to do this for every possible combination. Also, I think What should we do in this cases? |
I guess we should rely on the library doing the necessary registrations or on https://github.com/oracle/graalvm-reachability-metadata |
Wouldn't that only work when we have a resource? In this case (as in most configuration), the resources are optional. Would you still get an error if you try to register an optional resource (and look it up)? |
The registration works even if the resource is not present. It just marks the resource as "accessible", and if it's not there at run-time the program is supposed to behave as it would in JVM-mode. Note that this is going to affect all apps, not just Quarkus apps, so it's likely there will be some change that will relax the rules, but haven't seen anything like that happening yet. I will bring it up again upstream. Update: Upstream discussion oracle/graal#10264
No if you register an optional resource that is missing there will be no errors, neither at build time nor at run time. |
|
Ok, thanks for the clarification. Let me see what I can do on the SR Config side. |
|
So, I've completely disabled the classpath lookup for native mode. The problem is that when a user sets a config resource, we look it up in the classpath and in the filesystem, but we don't know where that will be. Because you can set these configurations at runtime (where the resource is not known), there is no way to register it properly, even if it is optional. I think it is still worth pursuing a way to exclude some calls from the exception. |
I don't see any updates on this branch, did you forget to push?
True, that's a limitation of the native compilation, you can't dynamically insert things in the classpath. So we could require any runtime set configuration to come from the filesystem. A problematic scenario with this would be the following: Assuming I have two configurations
Feel free to |
It is very small change:
Sure. |
SmallryeConfigBuildertries to access properties files from various sources, despite them not being available at run-time nor supported in native-mode, see #41994This patch also avoids getting a
MissingRegistrationErrorwhen using-H:+ThrowMissingRegistrationErrorsor--exact-reachability-metadata.Related to #41994 and #41995
Supersedes #42103