fix: removing the message about missing proactive closure - #51251
fix: removing the message about missing proactive closure#51251shawkins wants to merge 1 commit into
Conversation
closes: keycloak#51131 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Removes the warning about “missing proactive closure” and clarifies that request-end session closing may not always run (e.g., if the connection drops), relying on the CDI disposer as a safety net.
Changes:
- Update
CloseSessionFilterJavadoc to note it may not run when the connection is already closed. - Remove the
Loggerand warning log fromKeycloakBeanProducer.dispose()and always close the session there.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/jaxrs/CloseSessionFilter.java |
Adjusts filter documentation to reflect that it may not execute under some connection-closure scenarios. |
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/cdi/KeycloakBeanProducer.java |
Removes the proactive-closure warning and relies on disposer-driven closure as a fallback. |
ahus1
left a comment
There was a problem hiding this comment.
Thank you for looking into that. I've been reading the parent issue, and this is a rabbit hole.
I understand that the KeycloakBeanProducer is called in all place as the last line of defense.
Approving, but not merging yet in case you want someone else to review this one. Do you want to backport it as well?
Opting for removal, rather than trying to further refine this. What additional refinements would entail:
It doesn't seem like the additional complexity is warranted given this is already a corner case, and if we see a need for something like this in the future, it would be better to have quarkus provide us with a flag on the ResteasyReactiveRequestContext.
closes: #51131