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

Reorder Grpc server interceptors to apply exception handler correctly #39904

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

ozangunalp
Copy link
Contributor

Fixes #39602

To squash once reviewed

@ozangunalp ozangunalp requested a review from alesj April 5, 2024 10:56
@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Apr 5, 2024
@ozangunalp
Copy link
Contributor Author

@alesj I am not sure if this breaks anything. Please take a look

This comment has been minimized.

@alesj
Copy link
Contributor

alesj commented Apr 8, 2024

@ozangunalp so this looks like we had the interceptors order wrong all along?
@cescoffier ^^ ?!

@alesj
Copy link
Contributor

alesj commented Apr 8, 2024

@ozangunalp
Copy link
Contributor Author

@ozangunalp so this looks like we had the interceptors order wrong all along? @cescoffier ^^ ?!

Yes, the blocking interceptor schedules server listener calls on other threads so there wasn't any other way to apply the exception handling. The solution is to apply the blocking interceptor before the exception handling, so when an exception is thrown by the server method it gets handled by the handler and we close the server call by setting the status.

Where we need to pay attention in this change is all the other places I moved the list of interceptors. Sure tests pass but I am not sure whether we need to test something else.

@cescoffier
Copy link
Member

Yes, it may break users interceptor, but I think this change makes a lot of sense.

@ozangunalp
Copy link
Contributor Author

I am squashing it and it's good to go then

…request context interceptor but before exception handlers

Fixes quarkusio#39602
@alesj
Copy link
Contributor

alesj commented Apr 8, 2024

Yes, it may break users interceptor, but I think this change makes a lot of sense.

Break them in what way?

Copy link

quarkus-bot bot commented Apr 8, 2024

Status for workflow Quarkus CI

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

✅ 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.

@ozangunalp
Copy link
Contributor Author

Yes, it may break users interceptor, but I think this change makes a lot of sense.

Break them in what way?

Expecting to get called in event loop but getting called on worker thread. But at least the context will be preserved.

@alesj
Copy link
Contributor

alesj commented Apr 8, 2024

Expecting to get called in event loop but getting called on worker thread. But at least the context will be preserved.

Don't we / you control this with the @Blocking / @RunOnVirtualThread ... ?

Or this has changed now by default, and if someone is depending on this impl-details, it will break the current interceptors?

@ozangunalp
Copy link
Contributor Author

Expecting to get called in event loop but getting called on worker thread. But at least the context will be preserved.

Don't we / you control this with the @Blocking / @RunOnVirtualThread ... ?

It depends on the order in which interceptors are applied.

Or this has changed now by default, and if someone is depending on this impl-details, it will break the current interceptors?

I don't think it is a big deal as interceptors are still applied by priority.

@ozangunalp ozangunalp merged commit 9fc4e97 into quarkusio:main Apr 8, 2024
34 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 8, 2024
@ozangunalp ozangunalp deleted the block_grpc1 branch April 8, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants