Repository navigation
updates for quarkus:dev mode #47848
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
updates for quarkus:dev mode #47848
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Properties; | ||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.TimeoutException; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
|
|
@@ -178,6 +179,7 @@ public static Builder builder() { | |
| private List<Dependency> dependencies; | ||
| private boolean fipsEnabled; | ||
| private Properties systemProperties; | ||
| private CountDownLatch closed; | ||
|
|
||
| public Keycloak() { | ||
| this(null, Version.VERSION, List.of(), false); | ||
|
|
@@ -212,6 +214,8 @@ private Keycloak start(List<String> args) { | |
| return this; | ||
| } | ||
| StartupAction startupAction = action.createInitialRuntimeApplication(); | ||
| closed = new CountDownLatch(1); | ||
| startupAction.addRuntimeCloseTask(closed::countDown); | ||
| application = startupAction.runMainClass(args.toArray(new String[0])); | ||
|
|
||
| return this; | ||
|
|
@@ -304,6 +308,11 @@ private void closeApplication() { | |
| } catch (Exception cause) { | ||
| cause.printStackTrace(); | ||
| } | ||
| try { | ||
| closed.await(); // wait for an orderly completion of all cleanup in the other thread | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is a test framework class, it almost doesn't matter, but in general, I can't make sense of this, can you elaborate what it solves? Because the comment says:
However, the stack should be: So this only makes sense if the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It comes down to the cleanup I'm trying to do here: https://github.com/keycloak/keycloak/pull/47848/changes#diff-c1907932c84285e376fbc3a66277c655f30b91476db47834c3f5a81a7a45b319R89 - that is within the quarkus classloader after the application has been stopped. The quarkus classloader seems to be reused for dev mode reload - for example editing the command line. That logic also restores the previous system properties - we unfortunately make extensive use of setting those.
application.close() completes asynchronously from the cleanup being done in https://github.com/keycloak/keycloak/pull/47848/changes#diff-c1907932c84285e376fbc3a66277c655f30b91476db47834c3f5a81a7a45b319R89 There is further cleanup performed by the test logic when running in embedded mode - because it also modifies static state, including system properties. So I don't want these to trample each other. An alternative would be to only do the https://github.com/keycloak/keycloak/pull/47848/changes#diff-c1907932c84285e376fbc3a66277c655f30b91476db47834c3f5a81a7a45b319R89 cleanup if running in dev mode, but I was thinking that having a more sequential startup and shutdown for embedded may be good in general.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case it makes sense to add the startup task you are adding, I didn't realize there was the async exit. It doesn't eliminate every side-effect, but hopefully good enough for tests. |
||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
|
|
||
| QuarkusConfigFactory.setConfig(null); | ||
|
|
||
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.
It makes
configureConfigSourcesunused and never even called by Quarkus?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.
Corrected.