jakartaee / security

Jakarta Security
https://projects.eclipse.org/projects/ee4j.security
Other
47 stars 39 forks source link

Support async/reactive style auth mecanism #228

Open rmannibucau opened 2 years ago

rmannibucau commented 2 years ago

OpenID shows that current API is too rigid and, the typical example is the token fetch which is typically a NIO operation but is done synchronously in current implementation so misuse the server resources. Goal of this task is to ensure the API becomes more friendly to such needs. Minimum is to expect the SPI return types to be CompletionStage IMHO.

Side note: I don't think depending on a new dependency to be reactive with a lot of verbose API would be good for the project, sticking to java is sufficient IMHO.

arjantijms commented 10 months ago

is the token fetch which is typically a NIO operation but is done synchronously in current implementation so misuse the server resources.

With virtual thread support, would this mitigate the issue?

Minimum is to expect the SPI return types to be CompletionStage IMHO.

Where in the SPI exactly (which type, which method) would you like to see those?

Just for my understanding, this would only help when a reactive HTTP server is used right? For using e.g. Tomcat / Serlet there would be no benefit?

rmannibucau commented 10 months ago

With virtual thread support, would this mitigate the issue?

Well yes and no but it has some limits:

  1. assumes you run virtual threads everywhere
  2. enforces you to write blocking code - which is undesired in all but virtual thread case which is really something limited
  3. as of today virtual threads are a banalised forkjoin pool so you loose the bulkhead capabilities (which is quite bad IMHO)
  4. code can still block with virtual threads, in particular if you use any library

So overall I think it should have been thought the other way around even if it can be a temporary workaround sometimes today.

Just for my understanding, this would only help when a reactive HTTP server is used right? For using e.g. Tomcat / Serlet there would be no benefit?

Servlet is reactive since years (v3.0), agree it is maybe not integrated because the API does not enable it but since NIO connectors got adopted with AsyncContext this can be e2e reactive. It is also true for any application doing reactive style remoting (http calls, r2dbc etc) and needing a security layer but clearly i was more thinking about servlet container when creating the issue.

Note: the reactive part is really the callback nature more than the backpressure and things like that which can be hidden in a higher level or bridge to CompletionStage easily, this is why I think it would be bad to bring a reactive API or even go with Flow which is harder to integrate and will not benefit most applications IMHO.

arjantijms commented 10 months ago

Servlet is reactive since years (v3.0), agree it is maybe not integrated because the API does not enable it but since NIO connectors got adopted with AsyncContext this can be e2e reactive.

I see, so your goal is basically to free up the http request handling thread, and schedule all outstanding requests for token validations to a separate thread pool?

Of course the end user (caller) still has to wait for the result to eventually come back, and our server (the RP) still has to wait for the authentication server (the OP) to respond.

Practically speaking, you want to schedule this one on a thread pool / executor service:

https://github.com/eclipse-ee4j/soteria/blob/master/impl/src/main/java/org/glassfish/soteria/mechanisms/OpenIdAuthenticationMechanism.java#L338

And if it's indeed scheduled put the http request in async mode?

rmannibucau commented 10 months ago

@arjantijms the goal is to not have the scalabilty issue of the old blocking programming even when virtual threads are not an option. A simple example is when tomcat has 200 threads and you use blocking everywhere you can only get 200 connections if you have some latency in the system whereas you can go until >10k easily if you don't for example so yes the idea is to leverage the NIO eventing or similar when possible (more than the async which would stick to the same issue where you just fill yet another executor instead of using a #cores executor which is the ~max you can use on a machine).