Close the session within resteasy boundaries

[pedroigor]

Closes #15192

• Changes to QuarkusRequestFilter to avoid using Vert.x callbacks (e.g.: addHeadersEndHandler) to avoid closing sessions when the response is not yet done. We are still keeping this filter as it provides a very important hook for us to manage the entire request lifecycle and how it is dispatched for processing.
• Changes to KeycloakSession to make sure we can't close it more than once. I see improvements in this area in the future so that we can track the leaking of sessions (and underlying resources) and be more proactive about it.
• Introduced a TransactionalSessionHandler (right now basically a marker interface with some default methods) so that we can track the components responsible for managing the request/session lifecycle.
• The fact we are closing the session in multiple places is because we lack a proper hook from Quarkus. Another reason is that due to how Resteasy works on Quarkus we need to make sure the session is only closed after processing streamed responses (e.g: streams returned from endpoints).
• Also note that Vert.x/Quarkus developers assume everything is async. Because of that, we need to consider that closing sessions should not be done after the response is complete, otherwise, sub-sequent requests might see stale state. Now we have a lot of control over this and we are able to handle/control async/reactive responses. Right now everything is blocking though.

[ahus1]

Thanks for preparing this PR, I like the way it approaches things in a pure JAX-RS way. Please see below for some questions.

[pedroigor]

@ahus1 Thanks for your comments. I would like more input to see what we can be improving. Please, look at my last changes to the KeycloakSession.

[ahus1]

Hi @pedroigor - +1 for closing the session only once. Not sure though if theisClosed() should be exposed.

I saw that an earlier version threw exceptions whenever some method was called on an already closed session. I like that fail-safe design. I assume you removed it as it broke too many places? I'd be happy if we could have this in another PR, or maybe to start logging some warnings first.

[pedroigor]

@ahus1 I agree we should look at tracking session usage after closing it. We are really missing it.

However, I did not want to include the previous changes that you are referring to because they require a bit more time to get it right. For instance, need to consider async responses and delay closing sessions.

I do not see as a blocker but definitely something to improve. I have the changes almost ready and will create an issue and PR for your review.

I believe the close method should be part of the public contract. Are you concerned about people calling this method directly? Ifo wouldn't help the checks we are discussed around reusing a closed session?

[ahus1]

I believe the close method should be part of the public contract. Are you concerned about people calling this method directly? Ifo wouldn't help the checks we are discussed around reusing a closed session?

I agree with you that having this method would help with the transition. Still I think that any use of the method indicates that we're not sure if the code involved has either closed or not closed the session, which is a code smell indicating that we're not 100% sure what the code is doing.

So +1 to have it in the public interface for the transition.

[ahus1]

/rerun

[ahus1]

I reviewed the changes and tested them to my best knowledge and they look good to me. The failing model tests are due to a know unstable test.

I noticed one change that should go in the release notes:

• previously the REST calls have been dispatched to a Vert.x worker thread
• now they are dispatched to the executor pool

The new behavior is better, as I saw the RESTEasy stack sometimes dispatching the finishing response to the executor pool (didn't figure out the full scenario, but was probably a streaming response).

This requires uses to change their configuration on the thread pool:

• the old setting https://quarkus.io/guides/all-config#quarkus-vertx_quarkus.vertx.worker-pool-size which defaults to 20

• new setting https://quarkus.io/guides/all-config#quarkus-core_quarkus.thread-pool.max-threads that is at minimum 200 threads, but might be a lot more depending on the cores of the machine

IMHO that change is worth a text snippet in the release notes.

[hmlnarik]

Thank you @pedroigor for the PR!

While I like the overall direction a lot, I have to object adding a new method into KeycloakSession as it would expose internal state rather than keeping encapsulation principle. Please see inline for more details.

[hmlnarik]

I am not keen on extending the KeycloakSession with this method.

Prevention of throwing an exception is the sole reason for this method. The use of this method exposes KeycloakSession internal state. If KeycloakSession.close was idempotent and for active transactions handled the transaction commit / rollback itself, this could be completely omitted, and KeycloakSession.

[pedroigor]

Sorry, but I'm not following what you mean by exposing the state. Why exposing if the session is closed so bad?

We are really missing this as well as proper checks to enable a safer usage of the session.

[hmlnarik]

The code which handles the transaction is this and is found at many places across the codebase:

        KeycloakTransactionManager tx = session.getTransactionManager();

        try {
            if (tx.isActive()) {
                if (tx.getRollbackOnly()) {
                    tx.rollback();
                } else {
                    tx.commit();
                }
            }
        } finally {
            session.close();
        }

There is no place in codebase which closes the session and does not have the transaction commit handling code before itself in try-finally block.

This follows from the fact that closing session without committing the to-be-committed transaction is illegal: The changes in session would be lost, so the session should not exist in the first place.

Hence this code could and should be enclosed in KeycloakSession.close() instead of repeating it at many times differently, e.g. here or here. This enclosing also makes isClosed method redundant. One of the consequences is that e.g. rollback-only transactions are not handled at all e.g. in ScheduledTaskRunner

[stianst]

I agree with @hmlnarik here.

It would be much nicer if DefaultKeycloakSession#close handles the closing of the tx itself, so this boilerplate code isn't needed elsewhere.

DefaultKeycloakSession#close could just be a no-op if the session is already closed, or maybe it would be better to throw an exception? Strictly speaking a session that is closed should throw an exception if someone tries to use it.

[pedroigor]

I think throwing an exception makes more sense. I tried this approach and I like it because it gives a more defensive approach when managing the session lifecycle and quickly react when using it wrongly.

Regarding encapsulating the close/commit, that is the reason why I've added the TransactionalSessionHandler to remove boilerplate code between components related to creating/closing transactional sessions within the distribution codebase, without introducing more changes to the core codebase.

Still, I think we should expose whether or not a session is closed. We do have places where a session is only created to obtain providers without any transaction. For instance, setting up background tasks.

IMO, closing a session is purely about releasing providers/resources and not necessarily ending transactions. Keeping these two concepts separate is far more flexible than assuming a session is always transactional.

[hmlnarik]

Calling a session close twice is illegal operation, so an exception perhaps makes more sense.

Regarding encapsulating the close/commit, that is the reason why I've added the TransactionalSessionHandler to remove boilerplate code between components related to creating/closing transactional sessions within the distribution codebase, without introducing more changes to the core codebase.

I like the way of its separation from the rest of code, so +1 for this.

Still, I think we should expose whether or not a session is closed. We do have places where a session is only created to obtain providers without any transaction. For instance, setting up background tasks.

Controlling the "any transactions" part by the code is not possible as there is always transaction manager instantiated in a session which is never cleared:

keycloak/services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java

Line 94 in 1711782

this.transactionManager = new DefaultKeycloakTransactionManager(this);

Furthermore the code which uses the session has no access to actual transactions registered inside TM, so the commit / rollback operation is performed on TM and never on individual transactions directly, as shown e.g. here:

keycloak/services/src/main/java/org/keycloak/services/filters/AbstractRequestFilter.java

Line 64 in 1711782

tx.commit();

IMO, closing a session is purely about releasing providers/resources and not necessarily ending transactions. Keeping these two concepts separate is far more flexible than assuming a session is always transactional.

Should a provider be retrieved on an already closed session, it cannot read or write any data since the data transaction is committed, and - as also proposed in this PR - commit is never performed on an already closed session. Already closed session also contains potentially dirty transaction manager instance since that is never cleared (rightfully so). Thus I don't think that assumption of closing a session is purely about releasing providers/resources is correct. Supporting concept of reusing keycloak session as declared in the current codebase would thus be prone to errors.

[pedroigor]

I see.

I'm not talking about reusing closed sessions, that is what we are discussing avoiding, right?

My point is more about not assuming all usages of sessions are transactional. And we have this type of usage too.

I would prefer an API with semantics similar to what JPA and other frameworks do.

[pedroigor]

Btw, perhaps we can move discussions about this specific topic to #15223.

[pedroigor]

@ahus1 +1. I'm going to prepare the changes to the upgrade guide. In fact, we need to create specific options on our side to configure those tread pools.

Btw, created:

• Make sure the KeycloakSession is not closed more than once #15223
• Provide specific options for configuring the number of io and worker HTTP threads #15224

[pedroigor]

@ahus1 By using the quarkus thread pool we are also losing the capability from Vert.x to warn when a worker thread is blocking.

I think that is OK because it aligns better with what Resteasy extension does as it also dispatches to the same thread pool we are using.

[stianst]

@ahus1 +1. I'm going to prepare the changes to the upgrade guide. In fact, we need to create specific options on our side to configure those tread pools.

Btw, created:

• Make sure the KeycloakSession is not closed more than once #15223
• Provide specific options for configuring the number of io and worker HTTP threads #15224

I don't think this should be included in the release notes, as we haven't documented or suggested users change these settings. Anything users configure in quarkus.properties is considered unsupported, and as such should not be included in release notes or upgrade guides.

[pedroigor]

@ahus1 @hmlnarik @stianst I'm removing from the interface for now so that we can discuss the changes to KeycloakSession and its lifecycle on this issue #15223.

It should be safe (although not very clean) that we always have a single impl for KeycloakSession.

I'm pretty convinced we need this method as in the contract but it is not a blocker to moving forward and fixing the original issue.

[ahus1]

Thanks for preparing this PR and answering all my questions around it. Looking forward to the future issues, especially #15223

[hmlnarik]

Thank you @pedroigor for removing the isClosed from KeycloakSession.
I'm fine to have this in to unblock the release as long as the #15243 is also included soon.