-
Notifications
You must be signed in to change notification settings - Fork 41.2k
Add support for configuring embedded Jetty's max queue capacity #19494
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
Conversation
...ramework/boot/autoconfigure/web/reactive/ReactiveWebServerFactoryAutoConfigurationTests.java
Show resolved
Hide resolved
* @param <T> type of ThreadPool that the factory returns | ||
*/ | ||
@FunctionalInterface | ||
public interface JettyThreadPoolFactory<T extends ThreadPool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the team's stance on generics in general in Boot? I know I had added some previously in a different issue/pr but they were removed in a polish commit. I am not too strongly opinionated in this particular instance as it really only cleans up some casting from tests. The main area thats is used (EmbeddedJetty
s) does not take advantage of it and simply uses @ConditionalOnMissingBean(JettyThreadPoolFactory.class)
.
@Bono007 the issue you've referenced has the following:
What makes you think there isn't an equivalent for Tomcat or Undertow? I think the whole discussion on that thread is that whatever mechanism we introduce should be introduced consistently. |
Sorry for the lack of explanation. Here was my thinking:
Undertow's worker thread pool is unbounded but it is NOT currently configurable in Nio/XnioWorker.
There is an option on SummaryMechanisms to set the "max queue capacity" (which will need to be renamed something besides "queue capacity" at this point): Jetty: use our custom thread pool (this PR) Do we want to consolidate this functionality into this common property or handle the Tomcat and Undertow w/ documentation? For Tomcat its simple enough to set the other property. For Undertow though I could see doing a convenience default like I did for Jetty. The underlying mechanisms vary enough that trying to abstract something on top of them beyond this common property does not seem like a big win. Tomcat has this accept count. Jetty has this custom Proposal
|
@@ -36,10 +42,55 @@ public UndertowServletWebServerFactoryCustomizer(ServerProperties serverProperti | |||
this.serverProperties = serverProperties; | |||
} | |||
|
|||
// TODO why was this not being used before now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this Undertow specific customizer exists but was not being called anywhere. Will this be a problem?
I have created a 2nd commit that does the same type of resource limiting for Undertow. However, it is a completely different mechanism in Undertow than in Jetty and Tomcat. In those servers, its a thread pool with min/max thread count and backing queue that we limit the capacity on. However, in Undertow we don't have access to the queue in the blocking thread pool that it uses. The recommended method is to install a RequestLimitingHandler to constrain the requests/queued up callers. Additionally I think it only is available in Servlet environment (no DeploymentInfoCustomizer in UndertowReactiveWebServerFactory). I will look into this a bit more but wanted to get this code in for Undertow so reviewers could see the differences in implementation for each server. The main takeaway for me when comparing the 3 servers and the 3 mechanism to do this is that they are all different enough that I don't think it makes sense to try to unify them all under a single property. It would be artificial at best for Undertow specifically. SummaryJetty is solved w/ the previous commit and allows the following "dials":
Tomcat is already solved and allows the following "dials":
Undertow is the 2nd commit and allows the following "dials":
|
Thanks for the analysis, @Bono007. In light of what you've said, and the differences in how this works in Undertow and Jetty and Tomcat, I think we should just tackle Jetty (for now at least) using |
Thanks for the pr @Bono007. I've reduced the amount of change by making sure a |
Closes gh-19493
This is that pull request for Jetty. I think this property is Jetty specific and does not have a direct equivalent or current need in Tomcat or Undertow.
Approach
Create a
JettyThreadPoolFactory
interface that is used to set the backing thread pool on theJetty[Servlet|Reactive]WebServerFactory
.This was needed because the thread pool's backing queue (which is where the max capacity is set) can not be safely modified in a customizer as by the time the customizer is called, the server has already had its
QueuedThreadPool
and its backing queue implementation set during construction - its immutable after that.