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 detects multiple writers, but only one response type is present #14720

Closed
Postremus opened this issue Jan 31, 2021 · 14 comments · Fixed by #14748 or #16578
Closed

Resteasy Reactive detects multiple writers, but only one response type is present #14720

Postremus opened this issue Jan 31, 2021 · 14 comments · Fixed by #14748 or #16578
Labels
area/rest kind/bug Something isn't working
Milestone

Comments

@Postremus
Copy link
Member

Postremus commented Jan 31, 2021

Describe the bug
I have a GET method annotated with @Produces(MediaType.APPLICATION_JSON).
In the Score Console it says however that I have 2 writers for this method:

[io.quarkus.resteasy.reactive.jackson.runtime.serialisers.JacksonMessageBodyWriter@5a21df38, 
org.jboss.resteasy.reactive.server.providers.serialisers.ServerStringMessageBodyHandler@7a97f70c]

This negativly affects the score of this method. See screenshot.

Expected behavior
RR should not detect a simple Produces(MediaType.APPLICATION_JSON) as multiple writers.

To Reproduce

Steps to reproduce the behavior:

  1. Download the reproducer
  2. mvn quarkus:dev
  3. Go to http://localhost:8080/q/dev/io.quarkus.quarkus-resteasy-reactive/scores

Screenshots
image

Environment (please complete the following information):

  • Output of uname -a or ver:
    Linux martin 5.8.0-41-generic Add proper logging #46~20.04.1-Ubuntu SMP Mon Jan 18 17:52:23 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
  • Output of java -version:
    openjdk 11.0.7 2020-04-14 LTS
    OpenJDK Runtime Environment Zulu11.39+15-CA (build 11.0.7+10-LTS)
    OpenJDK 64-Bit Server VM Zulu11.39+15-CA (build 11.0.7+10-LTS, mixed mode)
  • Quarkus version or git rev:
    1.11.1.Final
  • Build tool (ie. output of mvnw --version or gradlew --version):
    Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
    Maven home: /home/martin/.sdkman/candidates/maven/current
    Java version: 11.0.7, vendor: Azul Systems, Inc., runtime: /home/martin/.sdkman/candidates/java/11.0.7-zulu
    Default locale: en_US, platform encoding: UTF-8
    OS name: "linux", version: "5.8.0-41-generic", arch: "amd64", family: "unix"
@Postremus Postremus added the kind/bug Something isn't working label Jan 31, 2021
@ghost ghost added the area/rest label Jan 31, 2021
@ghost
Copy link

ghost commented Jan 31, 2021

/cc @FroMage, @geoand, @stuartwdouglas

@geoand
Copy link
Contributor

geoand commented Feb 1, 2021

Does the GET method return String or something else?

@Postremus
Copy link
Member Author

@geoand yes, indeed it does in this example.
But I have also noticed this behavior when the GET returned a POJO.

@geoand
Copy link
Contributor

geoand commented Feb 1, 2021

Understood. I'll check it out soon

geoand added a commit to geoand/quarkus that referenced this issue Feb 1, 2021
…resolution of builders

By making certain writers extend AllWriteableMessageBodyWriter we can be certain that even in the presense
of multiple writers, RESTEasy Reactive can optimize itself to use only the first one

Fixes: quarkusio#14720
geoand added a commit to geoand/quarkus that referenced this issue Feb 1, 2021
…resolution of builders

By making certain writers extend AllWriteableMessageBodyWriter we can be certain that even in the presense
of multiple writers, RESTEasy Reactive can optimize itself to use only the first one

Fixes: quarkusio#14720
geoand added a commit to geoand/quarkus that referenced this issue Feb 2, 2021
…resolution of builders

By making certain writers extend AllWriteableMessageBodyWriter we can be certain that even in the presense
of multiple writers, RESTEasy Reactive can optimize itself to use only the first one

Fixes: quarkusio#14720
gsmet added a commit that referenced this issue Feb 2, 2021
Ensure that ServerStringMessageBodyHandler doesn't affect build time resolution of builders
@ghost ghost added this to the 1.12 - master milestone Feb 2, 2021
@gsmet gsmet modified the milestones: 1.12 - master, 1.11.2.Final Feb 2, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Feb 2, 2021
…resolution of builders

By making certain writers extend AllWriteableMessageBodyWriter we can be certain that even in the presense
of multiple writers, RESTEasy Reactive can optimize itself to use only the first one

Fixes: quarkusio#14720
(cherry picked from commit 2321d0c)
@kwakiutlCS
Copy link

I believe this regressed in 1.13.1.Final (same in 1.13.2.Final). In 1.13.0.Final it was ok.

@geoand
Copy link
Contributor

geoand commented Apr 15, 2021

I'll take a look tomorrow

@kwakiutlCS
Copy link

I created a small reproducer explaining what I did because I think this is for jackson only, with jsonb works good
https://github.com/kwakiutlCS/reactive-scores

@geoand
Copy link
Contributor

geoand commented Apr 16, 2021

Thanks

@geoand
Copy link
Contributor

geoand commented Apr 16, 2021

#16578 should fix the issue

geoand added a commit that referenced this issue Apr 16, 2021
Ensure that Jackson writer doesn't negatively affect endpoint score
@kwakiutlCS
Copy link

thanks @geoand. Can you clarify also if it is supposed for the score showing 0% when returning a Response object, because it says it only finds the writer in runtime?

I was expecting that
return entity;
and
return Response.ok(entity).build();
to be equivalent, but the scores are different. Is there really a performance difference in resteasy-reactive?

@geoand
Copy link
Contributor

geoand commented Apr 21, 2021

Yes, I can confirm that this is the intended behavior.

The problem is that because Response is not a generic type, RESTEasy Reactive can't know what Java type is being used for the payload at build time and therefore needs to resolve the MessageBodyWriter at runtime

@gsmet
Copy link
Member

gsmet commented Apr 21, 2021

@geoand I wonder if we should introduce a typed version of Response in RESTEasy Reactive with a proper type argument. It wouldn't be as flexible as you would define the type but I think it could work in a lot of cases.

@geoand
Copy link
Contributor

geoand commented Apr 21, 2021

Yes we have thought about it. If I am not mistaken, @FroMage had a branch with that

@FroMage
Copy link
Member

FroMage commented Apr 22, 2021

Yup. FroMage@bc744d9 But it's really a POC. I think it would make for a good feature, though. And since it's the second time other people than me have mentioned it, it's probably time to open an issue for it: #16718

@gsmet gsmet removed this from the 1.11.2.Final milestone Apr 26, 2021
@gsmet gsmet added this to the 1.13.3.Final milestone Apr 26, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Apr 26, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
5 participants