Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resteasy-reactive and rest-client: propagation of headers doesn't work #16059

Closed
Faboli7 opened this issue Mar 26, 2021 · 26 comments
Closed

Resteasy-reactive and rest-client: propagation of headers doesn't work #16059

Faboli7 opened this issue Mar 26, 2021 · 26 comments
Labels
area/rest area/rest-client kind/bug Something isn't working triage/wontfix This will not be worked on

Comments

@Faboli7
Copy link

Faboli7 commented Mar 26, 2021

Describe the bug

Hi, I have a production project using quarkus-resteasy(-jsonb) and quarkus-rest-client.

I wanted to give a try to the new quarkus-resteasy-reactive(-jsonb) extension so I replaced quarkus-resteasy-jsonb by this one in my pom.
Everything compiles without any change to the code or config and the app starts without error but I noticed that the headers are not propagated from my endpoints to the rest clients anymore.

For headers propagation I use the DefaultClientHeadersFactoryImpl (by annotating my clients with @RegisterClientHeaders) with the org.eclipse.microprofile.rest.client.propagateHeaders property.

EDIT: I tried to implement the behavior myself with a custom ClientHeadersFactory, but the header is also null in the incomingHeaders MultivaluedMap.

From my readings of this blog article, I assume that resteasy-reactive and rest-client should be compatible (and except this issue, it seems to work), so it looks like a bug.

Expected behavior

The headers that are part of the original request should be propagated to the rest client, like it was the case with quarkus-resteasy.

Actual behavior

The headers are not propagated.

To Reproduce

I could create a small reproducer but it doesn't seem necessary as I'm just using standard behavior. If you disagree, let me know and I'll add one.

Configuration

# Add your application.properties here, if applicable.
org.eclipse.microprofile.rest.client.propagateHeaders=Authorization

Environment (please complete the following information):

Output of uname -a or ver

Linux and Windows

Output of java -version

openjdk version "11.0.7" 2020-04-14 LTS
OpenJDK Runtime Environment 20.4-(Zulu-11.39+15-win_x64)-Microsoft-Azure-restricted (build 11.0.7+10-LTS)
OpenJDK 64-Bit Server VM 20.4-(Zulu-11.39+15-win_x64)-Microsoft-Azure-restricted (build 11.0.7+10-LTS, mixed mode)

GraalVM version (if different from Java)

Not using native builds

Quarkus version or git rev

1.12.2

Build tool (ie. output of mvnw --version or gradlew --version)

mvn 3.6.3

@Faboli7 Faboli7 added the kind/bug Something isn't working label Mar 26, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 26, 2021

@quarkus-bot quarkus-bot bot added the env/windows Impacts Windows machines label Mar 26, 2021
@geoand
Copy link
Contributor

geoand commented Mar 29, 2021

@michalszynkiewicz would you like to look into this?

@michalszynkiewicz
Copy link
Member

@geoand there's clearly some conflict between resteasy and resteasy-reactive. Users have reported such things in a few other places too, IIRC.
Do we want to support such a mix?

@geoand
Copy link
Contributor

geoand commented Mar 29, 2021

Well, ideally we would.

Practically, if there is something simple we can do to separate stuff without too much effort, that would be nice.

Otherwise we need to start making it very clear in docs (and perhaps a warning during the build), that users should use the new client.

@stuartwdouglas
Copy link
Member

Won't there be issues with all the JAX-RS providers if we have both on the ClassPath?

@geoand
Copy link
Contributor

geoand commented Mar 29, 2021

I thought this issue was only about quarkus-resteasy-reactive and the quarkus-rest-client being in the same application, which I would have hoped would work.
But if we think it won't work at all, we should start warning users.

@michalszynkiewicz
Copy link
Member

I'd rather we kept the RR and RestEasy classic worlds separate...
Anyway, I don't have the capacity to take a look at it now.

@Faboli7 would using rest-client-reactive be an option for you? The API is the same, nearly all features are covered. The drawback is that rest-client-reactive has just been added in Quarkus 1.13 so may be more buggy, and, until the quarkus platform is released, you'd have to use quarkus-bom instead of quarkus-universe-bom in your project's pom.

Even if it works for you, please don't close this issue, as @geoand wrote we should either make quarkus-rest-client work with resteasy reactive or warn users not to.

@Faboli7
Copy link
Author

Faboli7 commented Mar 29, 2021

Hi all and thanks for your replies.

@michalszynkiewicz The reasons why I used quarkus-rest-client is because I was already using it in my project and according to this paragraph it was supposed to work, and according to the second part of this paragraph, there was no alternative at the time of this article (except using quarkus-jaxrs-client but I don't want to), and still not in quarkus 1.12.
Of course when the resteasy-reactive version of the microprofile rest client is released, I'm willing to switch to it.

So I guess I'll wait for quarkus 1.13 to start testing the migration. Not a big deal.

@Faboli7
Copy link
Author

Faboli7 commented Apr 7, 2021

Hi,

I tried again with quarkus 1.13 and the new rest client extension but now I have issues with timeouts when I call external services.
From my logs and the logs of those services, I can tell that my application waits 30 seconds before actually making the call, and directly after I get a timeout.

Here is the partial stacktrace:

2021-04-07 18:13:15,706 INFO  [be.key.ins.api.fil.LoggingFilter] (vert.x-eventloop-thread-0) Received POST request to /auth/login
2021-04-07 18:13:46,129 ERROR [org.jbo.res.rea.ser.cor.ExceptionMapping] (vert.x-eventloop-thread-0)  Request failed : javax.ws.rs.ProcessingException: java.util.concurrent.TimeoutException
	at org.jboss.resteasy.reactive.client.impl.InvocationBuilderImpl.unwrap(InvocationBuilderImpl.java:199)
	at org.jboss.resteasy.reactive.client.impl.InvocationBuilderImpl.method(InvocationBuilderImpl.java:323)
...

2021-04-07 18:13:46,131 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-0) HTTP Request to /auth/login failed, error id: cf690a5c-b425-4f87-9100-e5337d46c05a-1: javax.ws.rs.ProcessingException: java.util.concurrent.TimeoutException
	at org.jboss.resteasy.reactive.client.impl.InvocationBuilderImpl.unwrap(InvocationBuilderImpl.java:199)
	at org.jboss.resteasy.reactive.client.impl.InvocationBuilderImpl.method(InvocationBuilderImpl.java:323)
...
2021-04-07 18:13:46,137 ERROR [org.jbo.res.rea.com.cor.AbstractResteasyReactiveContext] (vert.x-eventloop-thread-0) Request failed: javax.ws.rs.ProcessingException: java.util.concurrent.TimeoutException
	at org.jboss.resteasy.reactive.client.impl.InvocationBuilderImpl.unwrap(InvocationBuilderImpl.java:199)
	at org.jboss.resteasy.reactive.client.impl.InvocationBuilderImpl.method(InvocationBuilderImpl.java:323)

I'm not using async calls and I thought the issue might come from that so I tried to add @Blocking on my jax-rs resources classes but then it returns a 204 directly without even trying to call the services.

Any idea what's the issue ?
Shouldn't the rest client reactive extension works as a replacement of the "classic" rest client out of the box (since the resteasy-reactive guide sends the readers to the rest-client guide for that part) ?

I'm not very familiar with reactive programming (know the concept, never used it) so I might miss something but if I'm not mistaken, with this new extension we can still decide to use imperative or reactive pattern, am I right?

@geoand
Copy link
Contributor

geoand commented Apr 7, 2021

There are some enhancements coming in 1.13.1.
I would propose trying it when it comes out (likely by the end of the week) and if you still encounter problems, open a new issue

@Faboli7
Copy link
Author

Faboli7 commented Apr 7, 2021

Ok @geoand, I'll do that. Thanks.

@gorshkov-leonid
Copy link

gorshkov-leonid commented Apr 9, 2021

I am going to spend some time preparing an example that reproduces strange behavior that I found out after migration from 1.9.2 to 1.13.
So,

  1. I had an application where there was one place that is rest-service where I used the ThreadContext to wrap CompletableFuture-s and it used to work.
  2. Also I used CompletableFuture.completedFuture and CompletableFuture.allOf all over the code (I've just found that there is a completedFuture method on ManagedExecutor, but I have not found allOf there). And It seems wrappings were not required.
  3. Also I used Rest client with CompletionStage as returning the result.

But now after moving on 1.13 I found out that it is required to wrap any calls of rest client to context propagation. Also It gets required to wrap static methods CompletableFuture.completedFuture and CompletableFuture.allOf to get proper result as it worked earlier.

@geoand
Copy link
Contributor

geoand commented Apr 10, 2021

An example would be great, thanks

@gorshkov-leonid
Copy link

@geoand I prepared an example
https://github.com/gorshkov-leonid/quarkus113_header_propagation
See "v1-fail", "v2-fail, "v3-fail" and respective examples with "wa" suffix.
All tests are passed with quarkus 1.9.2 (see pom.xml), but these tests are failed with 1.13

@geoand
Copy link
Contributor

geoand commented Apr 11, 2021

Thanks

@gorshkov-leonid
Copy link

@geoand maybe it's better to create a separate ticket? I think it does not matter which is the environment.

@geoand
Copy link
Contributor

geoand commented Apr 13, 2021

I think it does not matter which is the environment.

What do you mean exactly?

@gorshkov-leonid
Copy link

Sorry for my English. I mean It makes no difference if it is Windows or not. I checked cases in a docker container. The result was the same. But I can see labels on this ticket. ... And I mixed up the words "issue" in github's terms with "ticket"

@geoand geoand removed the env/windows Impacts Windows machines label Apr 13, 2021
@geoand
Copy link
Contributor

geoand commented Apr 13, 2021

Understood, thanks!

I'll hopefully take a look at this soon

@geoand
Copy link
Contributor

geoand commented Apr 14, 2021

It looks like ResteasyContext.getContextData(HttpHeaders.class) is returning null even though org.eclipse.microprofile.rest.client.propagateHeaders=Authorization has been configured.

@michalszynkiewicz any idea what is going on?

@andreas-eberle
Copy link
Contributor

@geoand: I experienced the same issue. Luckily with 1.13.4 and the tipp to use the rest-client-reactive it worked for me.

Unfortunately, I wasn't able to find the quarkus-rest-client-reactive extension from googling before I knew its name and that it exists from this ticket. Also when I searched the reactive guide, I couldn't find it at first, because I searched for RestClient, which I hoped to find in a code example.

I think it could help others to improve the docs in this regard. Maybe add the segment about the reactive rest client also to the standard rest client documentation documentation and maybe write something about it in the getting started reactive guide.

Thanks for all your efforts!

@geoand
Copy link
Contributor

geoand commented May 19, 2021

Thank you for the input @andreas-eberle!

@michalszynkiewicz how do you feel about the proposed improvements?

@michalszynkiewicz
Copy link
Member

Adding info to the docs? 👍🏻

@geoand
Copy link
Contributor

geoand commented May 24, 2021

@andreas-eberle would you like to contribute that?

@gorshkov-leonid
Copy link

gorshkov-leonid commented May 26, 2021

As for the problem I described above with Context Propagation, RestEasy, and CF #17486

@geoand
Copy link
Contributor

geoand commented Jul 13, 2022

I am going to close this as this works properly when using RESTEasy Reactive and the Reactive REST Client

@geoand geoand closed this as not planned Won't fix, can't repro, duplicate, stale Jul 13, 2022
@geoand geoand added the triage/wontfix This will not be worked on label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest area/rest-client kind/bug Something isn't working triage/wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants