Skip to content

Conversation

pas2al
Copy link
Contributor

@pas2al pas2al commented Mar 27, 2019

Introduce the new callback interface TomcatProtocolHandlerCustomizer
that can be used to customize the ProtocolHandler on a Tomcat Connector.

Fixes #15035

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 27, 2019
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 28, 2019
@snicoll snicoll added this to the 2.2.x milestone Mar 28, 2019
@mbhave
Copy link
Contributor

mbhave commented Mar 28, 2019

@pas2al Thanks for the PR. I might be missing something but I don't see the new customizer being used anywhere in TomcatServletWebServerFactory and TomcatReactiveWebServerFactory. For example, the TomcatConnectorCustomizers get used here.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Mar 29, 2019
@pas2al
Copy link
Contributor Author

pas2al commented Mar 29, 2019

Can you elaborate on that?

@mbhave
Copy link
Contributor

mbhave commented Mar 29, 2019

Once the tomcatProtocolHandlerCustomizers have been set on the TomcatServletWebServerFactory and TomcatReactiveWebServerFactory, customize needs to be invoked for each one. This test will fail at the moment because the customize does not get called on the connector's protocol handler:

TomcatServletWebServerFactory factory = getFactory();
factory.addProtocolHandlerCustomizers(
        (TomcatProtocolHandlerCustomizer) protocolHandler -> protocolHandler.setThreadPriority(23));
Tomcat tomcat = getTomcat(factory);
Connector connector = ((TomcatWebServer) this.webServer).getServiceConnectors()
        .get(tomcat.getService())[0];
AbstractProtocol protocolHandler = (AbstractProtocol) connector.getProtocolHandler();
assertThat(protocolHandler.getThreadPriority()).isEqualTo(23);

@pas2al
Copy link
Contributor Author

pas2al commented Mar 30, 2019

@mbhave Ok, that makes sense. I missed that, sorry! So should i call customize for each ProtocolHandlerCustomizer like this?

for (TomcatProtocolHandlerCustomizer customizer : this.tomcatProtocolHandlerCustomizers) {
	customizer.customize((AbstractProtocol<?>) connector.getProtocolHandler());
}

Would be the customizeConnector method a good place to do that (you pinned it in your comment above)?

I tried it this way and confirmed it with the test. I just want to clarify that i am on the right track.

@mbhave
Copy link
Contributor

mbhave commented Apr 1, 2019

@pas2al Yup, customizeConnector would be a good place to do that. I also don't think we need to force the customizer to take a AbstractProtocol. We can just have the TomcatProtocolHandlerCustomizer customize a ProtocolHandler instead. It would look like this:

void customize(ProtocolHandler protocolHandler);

@pas2al
Copy link
Contributor Author

pas2al commented Apr 1, 2019

Ok, thanks for the feedback! Regarding the ProtocolHandler. In #15035, the description suggested to make it customizable like this:

@Bean
public TomcatProtocolHandlerCustomizer<AbstractHttp11Protocol<?>> processorCacheCustomizer() {
	return (handler) -> handler.setProcessorCache(250);
}

So passing the type to the TomcatProtocolHandlerCustomizer is still the way to go or do we customize a ProtocolHandler? But then the Bean example above will not work.

@mbhave
Copy link
Contributor

mbhave commented Apr 1, 2019

Sorry, you're right. I meant we could change the generic to expect a ProtocolHandler.

public interface TomcatProtocolHandlerCustomizer<T extends ProtocolHandler> {

That way we don't need to cast it to an AbstractProtocol when calling customize.

@pas2al
Copy link
Contributor Author

pas2al commented Apr 2, 2019

Ok, should i type usages of the TomcatProtocolHandlerCustomizer like this <TomcatProtocolHandlerCustomizer<ProtocolHandler>>? For example for the usages in the TomcatServletWebServerFactory or do you suggest anything else?

@mbhave
Copy link
Contributor

mbhave commented Apr 2, 2019

I think if you could update the PR with the change mentioned here and loop over the customizers to call customize for each one, we can take care of anything that's missing when merging.

Introduce the new callback interface TomcatProtocolHandlerCustomizer
that can be used to customize the ProtocolHandler on a Tomcat Connector.

Fixes #15035
@pas2al
Copy link
Contributor Author

pas2al commented Apr 2, 2019

I updated the PR.

mbhave pushed a commit that referenced this pull request Apr 4, 2019
This commit introduces a new callback interface that can
be used to customize the ProtocolHandler on a Tomcat Connector.

See gh-16342
@mbhave mbhave closed this in ad767ca Apr 4, 2019
mbhave added a commit that referenced this pull request Apr 4, 2019
* pr/16342:
  Polish "Simplify the configuration of the ProtocolHandler"
  Simplify the configuration of the ProtocolHandler
@mbhave
Copy link
Contributor

mbhave commented Apr 4, 2019

@pas2al Thank you for making your first contribution to Spring Boot. This is now merged into master along with this polish commit.

@mbhave mbhave removed the status: waiting-for-feedback We need additional information before we can continue label Apr 4, 2019
@mbhave mbhave modified the milestones: 2.2.x, 2.2.0.M2 Apr 4, 2019
@pas2al
Copy link
Contributor Author

pas2al commented Apr 4, 2019

@mbhave Thank you very much for the opportunity and your support!

@wilkinsona wilkinsona changed the title Simplify the configuration of the ProtocolHandler Provide a callback for customising Tomcat's ProtocolHandler Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Tomcat ProtocolHandlerCustomizer
4 participants