fix: refining startup, and adding a log on async start error#48733
fix: refining startup, and adding a log on async start error#48733shawkins wants to merge 3 commits intokeycloak:mainfrom
Conversation
| startup().run(); | ||
| } else { | ||
| ManagedExecutor executor = Arc.container().instance(ManagedExecutor.class).get(); | ||
| CompletableFuture.runAsync(startup(), executor).exceptionally(cause -> { |
There was a problem hiding this comment.
you can keep the startup signature and use
| CompletableFuture.runAsync(startup(), executor).exceptionally(cause -> { | |
| CompletableFuture.runAsync(this::startup, executor).exceptionally(cause -> { |
There was a problem hiding this comment.
I've added that change - it does broaden what is being run in an async manner, but that should be fine.
There was a problem hiding this comment.
It would probably be a good idea to add a javadoc, or throw an exception, about KeycloakApplication.getSessionFactory to warn that the instance won't always be available - and in general not to use it. I think after #48471 we'll have removed all of our non-test related usage of that method, so it's more of warning to those who may be trying to use services logic on their own.
| final var executor = Executors.newSingleThreadExecutor(); | ||
| CompletableFuture.runAsync(() -> runBootstrap(KeycloakApplication.sessionFactory), executor) | ||
| .exceptionally(throwable -> { | ||
| exit(throwable); |
There was a problem hiding this comment.
I had the idea that I logged the exception here. I'm surprised to see it missing.
closes: keycloak#48438 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
It's no trouble. Let me see if I can come up with a test for this, then move it out of draft. I tired to come up with a QuarkusTest, but that seemed didn't seem well-suited. @vramik if you are okay with it, I'd let this close #48438, then the user can open a new issue if there's an underlying migration problem that needs addressed. |
|
Should be ready for reivew now that there is a test. The exception handling is a little different between the async and non-async launches. The cli exception handler is in charge of the non-async exceptions, with the truncated presentation reminding users to specify --verbose. In async mode this pr is instead logging a fatal exception. For backport, I can pare down these changes, or if noone objects I'll just include everything. |
Does this mean the exception looks different (truncated vs full) based on the async mode? That is not very ideal. |
Yes, since I don't see a way to have quarkus treat the exception as if it were happening on the main thread, you have a couple of choices currently:
Longer term we could log a request for quarkus to support a call like Quarkus.asyncExit(code, throwable) - such that the throwable is forwarded to the Quarkus.run exit handler. |
What would be the estimated effort for this? I'd really want to avoid inconsistent error handling. Potentially it could be a follow-up. |
Mostly just ugly. We are crossing over something from picocli to quarkus, so that means we need to use a static variable. |
|
@shawkins can't you fetch the On I think we can merge this as is and revisit it later. |
The place we actually want it is KeycloakMain.start - one way or the other that would currently require a static variable. We could think about going the other direction - making the non-async handling more like this pr. There's really no reason to supply --verbose to see the exceptions happening at this point. Anything past the session factory init will be something that's best to already have the stack trace for. |
Doing that causes a couple of test failures: https://github.com/shawkins/keycloak/actions/runs/25582732362/job/75105599369 That is a little verbose for something like a wrong db password. I'm open to doing that or I've opened |
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: #48438
@pruivo the analysis of @vramik is correct, we're at least missing a log on an async startup error.
For the backport that could be a one line change, but moving forward I'd like to consider doing something like this PR to further simplify how we're doing the async starts. There is a lot of coupling between what thread to use (I think we should be using a managed one for full continutity with non-async starts), the exit handling, and the determination of whether async should be used, such that it would be clearer just to have all that logic in the same place.
Not added yet, but to test this scenario currently we either need a failing migration or to add a custom provider via the quarkus test logic that fails in postInit.