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

Auto-configured customizers are stil applied on a user-defined ServletWebServerFactory #24574

Closed
plilja opened this issue Dec 19, 2020 · 4 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@plilja
Copy link

plilja commented Dec 19, 2020

This bug occurs if you are using Jetty and Spring Boot version 2.4.1.

If you set a custom ThreadPool on your JettyServletWebServerFactory, then that thread pool was used for http requests in 2.2.0. After upgrading to version 2.4.1 that thread pool is no longer being used.

I.e. somewhere in your setup you have this code and you would expect the thread pool to be used.

    @Bean
    JettyServletWebServerFactory jettyServletWebServerFactory(QueuedThreadPool queuedThreadPool) {
        JettyServletWebServerFactory jettyServletWebServerFactory = new JettyServletWebServerFactory();
        jettyServletWebServerFactory.setThreadPool(queuedThreadPool); // Customized thread pool
        return jettyServletWebServerFactory;
    }

I've created a sample project that replicates the bug:
https://github.com/plilja/spring-boot-threadpool-bug
You can switch Spring boot version in that project to see that this worked in 2.2.0.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 19, 2020
@snicoll
Copy link
Member

snicoll commented Dec 21, 2020

Thanks for the report.

As of Spring Boot 2.3, we are auto-configuring thread-pool related information, see #19475 and the release notes. Assuming that's all you have in your app, you can remove that code and use the server.jetty.threads.* namespace to configure your thread pool.

Also, exposing a JettyServletWebServerFactory is meant to take control over the registration of the server, not configure one aspect of it. Here is a change to your sample that makes it work (and that is more aligned with what you should be doing according to the reference guide).

@Bean
WebServerFactoryCustomizer<JettyServletWebServerFactory> jettyThreadPoolCustomizer(QueuedThreadPool threadPool) {
	return (factory) -> factory.setThreadPool(threadPool);
}

With all that being said, I'd like to keep this issue open to investigate how we could potentially react to the fact a thread pool has already been set (and backoff our customisations). I also think that this section of the doc may need a refresh as I was expecting all the customizers to back off if we create our own factory and they didn't.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Dec 21, 2020
@plilja
Copy link
Author

plilja commented Dec 21, 2020

Thanks for the info @snicoll ! Yes we will be switching to the server.jetty.threads-properties. So that solves our problem.

I reported it since it's a little bit hard to spot that this happened. The application starts just fine but your custom configuration is no longer present.

@snicoll
Copy link
Member

snicoll commented Dec 21, 2020

I reported it since it's a little bit hard to spot that this happened.

Yup, and thank you again for doing that. A customizer runs after ours by default so the code I've shared in my previous comment is the idiomatic way to customise a server and yet benefit from the rest of the auto-configuration. We've started to chat about this issue and we'll try to improve it so that it is less surprising.

@snicoll snicoll changed the title Jetty: ThreadPool set in JettyServletWebServerFactory isn't being used Auto-configured customizers are stil applied on a user-defined ServletWebServerFactory Dec 21, 2020
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jan 6, 2021
@snicoll
Copy link
Member

snicoll commented Jan 8, 2021

@plilja we're going to update the reference guide to insist that one should not expose a WebServerFactory to configure part of the infrastructure (#24705). We'll also revisit what happens when you do (#24706).

This issue is now superseded by those two. Thanks again for the report.

@snicoll snicoll closed this as completed Jan 8, 2021
@snicoll snicoll removed their assignment Jan 8, 2021
@snicoll snicoll added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

4 participants