RESTEasy Reactive: ContainerRequestContext getEntityStream returns empty stream

[bcluap]

Describe the bug
The RESTEasy reactive module does not obey the JAX-RS Spec with respect to accessing the entity steam in filters:

"ContainerRequestContext is a mutable class that provides request-specific information for the filter,
such as the request URI, message headers, message entity or request-scoped properties."

Expected behavior
Accessing ContainerRequestContext getEntityStream should return a reference to the inputstream for the body of the request.

Actual behavior
The method returns a org.jboss.resteasy.reactive.common.util.EmptyInputStream which has an empty stream

To Reproduce

git clone https://github.com/bcluap/quarkus-examples.git
cd quarkus-examples/resteasy-reactive
mvn clean test

Configuration
Nothing specific

Environment (please complete the following information):
Nothing specific

Additional context
The reactive module does this in order to prevent code from blocking the eventloop thread if it attempts to read the body. This is understandable but there probably needs to be a means of doing this in order to allow for migration of normal JAX-RS module to the reactive version and be able to implement this part of the spec. There are 2 scenarios we use this for and neither currently works in resteasy reactive:

1. Sometimes a pre or post matching filter needs to actually process the incoming request and reply to it. For example, we have an authentication filter which intercepts calls to /login, /renew, /logout and processes them using configurable auth providers. These providers need to read the message body.

2. A common use of filters is to log/audit incoming messages. For example, an XSRF filter which checks that the body does not contain script tags and the like. These filters would typically do this:
   responseContext.setEntityStream(new MyPiggyBackStreamReader(responseContext.getEntityStream()));
   Then as the stream is read, it can process the chunks and log the request, look for blocked tokens etc etc.

Now, scenarios 1 and 2 are different in that scenario 1 will likely end up blocking the eventloop for large bodies, while scenario 2 does not block anything - it just copies and processes the body as it is being read in a non-blocking fashion.

Solution for scenario 2
I believe that one should be able to achieve scenario 2 without having to block. I think that getEntityStream() should return the actual stream but with a caveat that any attempt by an eventloop thread to read the stream when there are no bytes available without blocking, would throw an exception much like trying to block without adding @Blocking to a rest resource.

Possible solution For Scenario 1
If any pre or post matching filter class is annotated with @Blocking then the entire filter pipeline should be called with an executor thread and attempts to use getEntityStream and read in a blocking fashion should be allowed. All subsequent JAX-RS resources will then also be called with an executor thread - irrespective of their annotations or willingness to use the eventloop. This in effect makes the entire application work with the executor pool.

Another Possible solution For Scenario 1
A pre or post matching filter can cast the request context to the specific implementation if it is known to be running in reactive mode, and then call a specific method (like one does with suspend()) to indicate that all filters / resources from then on for the request pipeline should be called with an executor thread (i.e. go into blocking mode).
((ContainerRequestContextImpl) requestContext).blocking();
One could thus write a filter which can dynamically decide when to use blocking or non blocking mode on a per request basis by looking at the path, headers etc. If in blocking mode, then attempts to use getEntityStream and read in a blocking fashion should be allowed.

If I could have my cake and eat it then perhaps all 3 changes could be done, or maybe change 1 and 2 at first and then the last one later down the line if there is support for it.

[ghost]

/cc @FroMage, @geoand, @stuartwdouglas

[geoand]

This is an interesting problem.

I like the idea of allowing a filter to deciding whether it's blocking or not, but I am not sure I would like to advertise downcasting to a specific type.

Maybe the following would make more sense:

When using @ServerRequestFilter, allow Uni<Void> as a return in addition to void. The idea here is that RESTEeasy Reactive will automatically suspend before calling the filter and resume when the Uni completes. RESTEeasy Reactive already supports this for @ServerExceptionMapper.

Now in addition to that, we need a way for the filter to denote that is needs to have access to the entire body. I am thinking that we could introduce a new annotation that one would use on the method already annotated with @ServerRequestFilter. Or maybe a boolean value on the existing @ServerRequestFilter annotation.

@stuartwdouglas WDYT?

[bcluap]

At what point do you hand over to an executor thread if the resource is @Blocking? E.g. let's say I set @Blocking on my application class (FYI - never managed to get this to work BTW) so all resources are blocking, then would my pre and post matching filters run in an executor?

If they do, then you might as well allow access to the entire body if you see that you are in an executor. This way one can get an existing application to act just like standard resteasy just by adding @Blocking to the application class.

[geoand]

At what point do you hand over to an executor thread if the resource is @Blocking? E.g. let's say I set @Blocking on my application class (FYI - never managed to get this to work BTW)

I recently fixed a bug in this.

If they do, then you might as well allow access to the entire body if you see that you are in an executor. This way one can get an existing application to act just like standard resteasy just by adding @Blocking to the application class.

Yes, when a method is @Blocking, the filters also run on the executor instead of the event loop.

I see what you mean, I am just thinking that we probably want to have a declarative way to control the desired behavior (as the one I described above), instead of relying on the semantics of Blocking which could end up confusing people.

[bcluap]

Ok great. In terms of a declarative way - my sense is that declaring @Blocking on the application is sufficient to say that you want it to act like normal RESTEasy but with the performance and reengineering benefits of the Reactive codebase. The Reactive implementation is faster and nicer to use even in blocking mode!

I see what you mean, I am just thinking that we probably want to have a declarative way to control the desired behavior (as the one I described above), instead of relying on the semantics of Blocking which could end up confusing people.

Why would you not allow a filter to access the body if the application has said that it's blocking? Is another annotation really necessary cause if any filter has this new annotation/option then effectively the entire app becomes blocking - so now 2 different annotations result in the same thing...

[geoand]

The problem is this the way I see it:

Currently, a JAX-RS Resource method is blocking, not an entire application. If we end up adding @Blocking to filters as well, then that means we automatically make every JAX-RS Resource blocking as well (which can both be confusing and potentially unnecessary for some)
With my proposal, the body can be read in a non-blocking blocking fashion and the endpoints continue to decide whether they need to be blocking or not.

[bcluap]

Ok sorry, I think I confused things.

my sense is that declaring @Blocking on the application is sufficient to say that you want it to act like normal RESTEasy but with the performance and reengineering benefits of the Reactive codebase

By that, I mean @Blocking on the class that extends Application. I.e. I am declaring that I want all resources and filters to run on the executor thread. By doing so, should this not also allow access to the body by default as well?

Then, in addition, your proposal can be used in case someone wants to access the body and they have not set @Blocking at the application level.

[FroMage]

Don't we have an API to read the body in an async way?
And, what do we do with the body if a filter reads it? Can the resource then later re-read it? If yes it needs to be buffered, no?

[bcluap]

There are 2 scenarios at play when a filter accesses the body:

1. The filter wraps the stream so that it can buffer the body and do whatever it needs while the resource is reading it. This is transparent to the resource.
2. The filter directly reads the inputstream itself. This would most likely also need to be done with a .suspend() call to tell the platform that it should not try and process the request any further.

I cannot see a scenario where someone would consume the body themselves and then still allow the destination resource to try and process the request - cause as you say, the stream will have been consumed. Unless someone reads the stream and then replaces it with a ByteArrayInputStream containing the data they read. But that's not ideal as it would negate the benefits of streaming in case the body is large (would need to sit in memory).

[geoand]

Ok sorry, I think I confused things.

my sense is that declaring @Blocking on the application is sufficient to say that you want it to act like normal RESTEasy but with the performance and reengineering benefits of the Reactive codebase

By that, I mean @Blocking on the class that extends Application. I.e. I am declaring that I want all resources and filters to run on the executor thread. By doing so, should this not also allow access to the body by default as well?

Then, in addition, your proposal can be used in case someone wants to access the body and they have not set @Blocking at the application level.

Right, I see what you mean

[geoand]

Don't we have an API to read the body in an async way?

I don't think so. This sort of scenario hasn't really come up.

[bcluap]

Any update on this issue. So keen to move to resteasy reactive. Just this issue blocking me. No pun intended

[geoand]

I haven't had time to look into this, but hopefully will soon

[geoand]

Any update on this issue. So keen to move to resteasy reactive. Just this issue blocking me. No pun intended

This comment is worthy of a tweet 😎

[geoand]

It seems like this was already fixed in #14783.

What it does is what I had in mind - when there is no InputStream, create a blocking one.
This is actually a little dangerous and should only be used on @Blocking endpoints, but it should cover your use case.

Moreover, 1.12.0.CR1 should be available today or tomorrow.

[bcluap]

Great thanks. I'll add @Blocking on the application level and give this a go.

…

On Thu, Feb 11, 2021 at 10:38 AM Georgios Andrianakis < ***@***.***> wrote: It seems like this was already fixed in #14783 <#14783>. What it does is what I had in mind - when there is no InputStream, create a blocking one. This is actually a little dangerous and should only be used on @Blocking endpoints, but it should cover your use case. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14554 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGMB23KBGMV3M7VBF7KAJFDS6OJRVANCNFSM4WQQNGFQ> .