-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix smallrye ConfigValidationException not being handled properly at Quarkus startup #37543
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Here are a few comments. I think the changes to generated code, in particular, could be made a bit safer... This is very impacting code :)
core/deployment/src/main/java/io/quarkus/deployment/steps/RuntimeConfigSetupBuildStep.java
Outdated
Show resolved
Hide resolved
core/runtime/src/main/java/io/quarkus/runtime/ApplicationLifecycleManager.java
Outdated
Show resolved
Hide resolved
core/runtime/src/main/java/io/quarkus/runtime/ApplicationLifecycleManager.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java
Outdated
Show resolved
Hide resolved
@yrodiere I changed the things from your review, please have a look now :) |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking good to me.
I rebased to avoid the merge commit, and marked as draft while we try to figure out the test failures.
I wasn't able to check this works locally, because an unrelated regression on main is preventing my reproducer from starting. I'll give it another try and will report back.
Thanks for Your help, yeah yesterday I wanted to sync up with main by pulling new changes and by mistake I clicked "Update branch" on Github which merged main into my branch... |
So I tried to test this locally, unfortunately I can't because of a different regression: #37193 (comment) Will try again later, otherwise I'll have to ask someone else to approve.
Thanks. The failing tests mention "Validation" failures, so it looks related, but it might just be that they expect a different error message to be produced, in which case updating the expectations could be fine. |
This comment has been minimized.
This comment has been minimized.
Checked those tests, issues occur both at main and mine branch, so it seems like some regression (not the issue related to my changes). |
While Did the test pass locally for you? |
You are right, when I tested |
Yes, this is a telltale sign that multiple classloaders are involved, each with its own definition of TBH I'm pretty sure there's a problem with classloading in The question is: which If the If the |
Thanks for explanation, now I understand why this test was it failing (this is something new for me). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This comment has been minimized.
This comment has been minimized.
Thanks! If there is anything more that I have to do in order to merge this please let me know :) |
Well now that you mention it... there seem to be new tests failing with your latest changes :x I just force-pushed to your branch to rebase on the latest |
This comment has been minimized.
This comment has been minimized.
I had a look into why they were failing and it was due to the same reason - it was checking in tests if throwable was an instance of |
The build just finished and I see that now different tests having the same |
Agreed, that was a reasonable strategy with only one or two tests failing, but not if it means patching the whole codebase.
I just had a look at the code, and FWIW there is a way to handle
That means you might be able to to Now, obviously, we don't want to change all tests that do an What I'm wondering is why all those tests are failing... the only thing you did is to wrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This could probably be made a bit more future-proof (see comment), but this seems relatively safe from side-effects, so let's see if it works :)
That being said, if we have to go through such hoops, we might want to reconsider whether instanceof
really is the right way to do this... I mean, it's entirely possible that e.g. the Hibernate ORM extension would throw a ConfigurationException
when it starts, because some configuration is incorrect, and in that case it would be perfectly safe to display the current config profile.
Let's step back a bit: the point of this detection code is to detect whether ConfigUtils.getProfiles()
will return a correct value. But we can't really know here, because we don't know how far the application managed to boot.
I think I should probably open a follow-up issue to implement some kind of listener (similar to ApplicationStateNotification
-- maybe we could use that?) to register the information "the application managed to create and validate configuration, error reporting code can use it safely". And we'd get rid of this attempt at guessing this information from very incomplete data.
core/runtime/src/main/java/io/quarkus/runtime/ApplicationLifecycleManager.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Thanks, this looks good to me. Let's see what CI has to say, and merge if it's happy. |
Fingers crossed :D Thanks @yrodiere for all the work and help! |
This comment has been minimized.
This comment has been minimized.
The other failures are definitely unrelated, but I'm not sure about the one in oidc-client... See the stack trace below, it's unclear. I couldn't find a similar failure in other PRs, though... I'll rebase and attempt another build, to see if it was just a flake.
|
…operly at Quarkus startup
BTW @MaciejDromin I'll squash your changes, too. Hope you don't mind. |
No I don't mind at all, please do. Also thanks for checking this test - yeah stack trace looks unclear but I'm not sure it is due to my changes. Let's wait for build if it fails again I will investigate what is happening - at least I will try to :D |
✔️ 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. |
@yrodiere Now it is all green, so it must have been a flaky test after all :D |
Yep. Merging, thanks! |
It fixes #36376
It was needed to wrap
io.smallrye.config.ConfigValidationException
exception withio.quarkus.runtime.configuration.ConfigurationException
And then as it turned out every exception thrown by
RuntimeConfigSetup#deploy
method was automatically wrapped into RuntimeException which laterApplicationLifecycleManager
could handle. Screenshot shows how it is now behaving.