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

Allow the use of @Blocking on @ClientExceptionMapper #40721

Merged
merged 1 commit into from
May 20, 2024

Conversation

geoand
Copy link
Contributor

@geoand geoand commented May 20, 2024

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 20, 2024
@cescoffier
Copy link
Member

I believe none of the other extension handling @Blocking handle this case.

@ozangunalp do you know about reactive messaging?
@alesj do you know about gRPC?
@mkouba do you know about websocket next?

@geoand
Copy link
Contributor Author

geoand commented May 20, 2024

That's a very good point, however the REST client might be unique in that the various Providers are run on the event loop by default (which was probably a mistake)

@mkouba
Copy link
Contributor

mkouba commented May 20, 2024

@mkouba do you know about websocket next?

@cescoffier if you ask whether we produce an ExecutionModelAnnotationsAllowedBuildItem for endpoint callbacks then yes, we do: https://github.com/quarkusio/quarkus/blob/main/extensions/websockets-next/deployment/src/main/java/io/quarkus/websockets/next/deployment/WebSocketProcessor.java#L135-L145 :D

@ozangunalp
Copy link
Contributor

For messaging, we only detect incoming/outgoing annotated methods on beans and not from superclasses.

Copy link

quarkus-bot bot commented May 20, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9b72af5.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit 1d56193 into quarkusio:main May 20, 2024
26 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 20, 2024
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 20, 2024
@geoand geoand deleted the #38275 branch May 20, 2024 09:06
@alesj
Copy link
Contributor

alesj commented May 20, 2024

@cescoffier is gRPC exception handler is a plain ServerCall.Listener impl, which I guess could work with blocking, if there is a @Blocking before on the method

@cescoffier
Copy link
Member

Ok, so, messaging and gRPC - only for the concrete method. WS and REST: super-method too.

We may need to make this a bit more homogenous (BTW does @Blocking has the inherent meta-annotation?)

@mkouba
Copy link
Contributor

mkouba commented May 20, 2024

Ok, so, messaging and gRPC - only for the concrete method. WS and REST: super-method too.

Hm, I'm not quite following. WS Next does not support class-level @Blocking/@NonBlocking atm.

We may need to make this a bit more homogenous (BTW does @Blocking has the inherent meta-annotation?)

Keep in mind that @Inherited should only be considered for class-level annotations. And no, neither @Blocking nor @NonBlocking is annotated with @Inherited.

@gsmet gsmet modified the milestones: 3.12 - main, 3.11.0 May 21, 2024
@geoand geoand changed the title Allow the of @Blocking on @ClientExceptionMapper Allow the use of @Blocking on @ClientExceptionMapper Aug 7, 2024
@gsmet gsmet modified the milestones: 3.11.0, 3.8.6 Aug 14, 2024
fedinskiy added a commit to fedinskiy/quarkus-test-suite that referenced this pull request Aug 22, 2024
fedinskiy added a commit to fedinskiy/quarkus-test-suite that referenced this pull request Aug 27, 2024
jcarranzan pushed a commit to quarkus-qe/quarkus-test-suite that referenced this pull request Aug 27, 2024
jedla97 pushed a commit to jedla97/quarkus-test-suite that referenced this pull request Sep 3, 2024
rsvoboda pushed a commit to quarkus-qe/quarkus-test-suite that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants