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

Produce a 503 response when there no more worker threads to handle blocking requests #36869

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

cescoffier
Copy link
Member

Fix #36860

@cescoffier cescoffier requested a review from geoand November 4, 2023 09:07
@cescoffier
Copy link
Member Author

I tested it manually. I didn't want to implement a test as it was going to be flaky. You need to overload the server, and on GH actions this is
1 - not necessarily easy to control
2 - can have unexpected consequences

The code is self-explanatory.

@ahus1
Copy link
Contributor

ahus1 commented Nov 4, 2023

@cescoffier - I see a System.out.println in the code, and I assume this might be a left-over code snippet from a debugging session that should be removed from the PR before merging it.

https://github.com/quarkusio/quarkus/pull/36869/files#diff-085d25b0aecabee93538a3b07042b3d26610fd0ffc7e0fa50ee35031ca995361R53

Besides that, I've reviewed it in the context of Keycloak, and the solution should work fine handling the exception thrown using RESTEasy reactive.

@cescoffier
Copy link
Member Author

The exception is not handled in RestEasy reactive, but at the vertx http level directly. Thus it will work with RestEasy classic, reactive routes, gRPC...

@cescoffier
Copy link
Member Author

System.out removed.

@ahus1
Copy link
Contributor

ahus1 commented Nov 4, 2023

@cescoffier - let me rephrase my previous statement: "The solution you are presenting here looks good and will work fine with Keycloak's use of RestEasy reactive". The only reason I used "should" was that I was just debugging the code, and not testing a full backport of change with Keycloak. Sorry for the confusion.

So TL;DR: This solution is fine for Keycloak

Copy link

quarkus-bot bot commented Nov 4, 2023

✔️ 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.

@cescoffier cescoffier merged commit 00074a4 into quarkusio:main Nov 4, 2023
50 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Nov 4, 2023
@cescoffier cescoffier deleted the fix-36860 branch November 5, 2023 06:42
@gsmet gsmet modified the milestones: 3.6 - main, 3.5.1 Nov 6, 2023
@aloubyansky aloubyansky modified the milestones: 3.5.1, 3.2.8.Final Nov 7, 2023
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.

Exhausted thread pool returns error 500 instead of 503 status code
5 participants