Repository navigation
updates for quarkus:dev mode - #47848
Conversation
| @@ -769,40 +767,10 @@ private void configureUserDefinedPersistenceUnits(List<PersistenceXmlDescriptorB | |||
| * | |||
| * @param configSources | |||
| */ | |||
| @BuildStep( ) | |||
There was a problem hiding this comment.
It makes configureConfigSources unused and never even called by Quarkus?
ef2c034 to
76be4d4
Compare
|
Now ready for a review |
| @@ -307,6 +311,11 @@ private void closeApplication() { | |||
| } catch (Exception cause) { | |||
| cause.printStackTrace(); | |||
| } | |||
| try { | |||
| closed.await(); // wait for an orderly completion of all cleanup in the other thread | |||
There was a problem hiding this comment.
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:
// wait for an orderly completion of all cleanup in the other thread
However, the stack should be: org.keycloak.Keycloak#closeApplication -> io.quarkus.bootstrap.app.RunningQuarkusApplication#close -> closeable from io.quarkus.runner.bootstrap.StartupActionImpl#runMainClass which runs these close tasks where you added CountDownLatch. And they are not strictly speaking the last to run (e.g. there is RuntimeApplicationShutdownBuildItem and few other actions). That may be irrelevant since we shouldn't rely on the impl. details anyway.
So this only makes sense if the io.quarkus.bootstrap.app.RunningQuarkusApplication#close has some asynchronous tasks, otherwise regardless of the thread switching it should only end the close invocation when all the tasks are done. If it doesn't work this way, I think it is dangerous considering all the class loader switching they are doing.
There was a problem hiding this comment.
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:
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.
So this only makes sense if the io.quarkus.bootstrap.app.RunningQuarkusApplication#close has some asynchronous tasks
application.close() completes asynchronously from the cleanup being done in https://github.com/keycloak/keycloak/pull/47848/changes#diff-c1907932c84285e376fbc3a66277c65 10000 5f30b91476db47834c3f5a81a7a45b319R89
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
LGTM (considering also @michalvavrik's deeper review). Thanks!
closes: keycloak#47077 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # quarkus/tests/junit5/src/main/java/org/keycloak/Keycloak.java
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
* fix: ensuring dev mode can fully reload the application closes: keycloak#47077 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * also removing test launch mode Signed-off-by: Steve Hawkins <shawkins@redhat.com> * refining embedded mode cleanup Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # quarkus/tests/junit5/src/main/java/org/keycloak/Keycloak.java * restoring test launch mode Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: #47077
removed test launch mode in favor of reusing non-server mode andadded docs for some Environment methods.