Skip to content

Common way to configure HTTP/2 #9981

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

Closed
wants to merge 14 commits into from
Closed

Conversation

pvorb
Copy link
Contributor

@pvorb pvorb commented Aug 8, 2017

This PR is an attempt to provide a solution to #3350 that tries to be Servlet Container agnostic.

This PR is currently still in progress. Yet, I'd like to collect some thoughts on the approach taken, since PR #3904 already got closed.

My approach is to introduce a new property server.http2.enabled=true and then customize Tomcat, Undertow and Jetty to enable HTTP/2 in the DefaultServletWebServerFactoryCustomizer. There, each Servlet Container can be configured in their own way.

I could also think of having a similar property somewhere under spring.http.. If I understand the logic of Spring Boot's properties right, as this property doesn't change the behavior of Spring directly, having it under server.http2. is better.

Please do not build upon this PR as I'm going to rebase onto the latest master and maybe fix issues in old commits as I proceed. Also, it's not fully functional as of now.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 8, 2017
@pvorb pvorb force-pushed the feature/http2 branch 2 times, most recently from 1d54dc1 to bb36b9d Compare August 8, 2017 22:50
@wilkinsona
Copy link
Member

Thanks for the PR. Please be aware that we have another PR that proposes to enable HTTP/2 by default with Tomcat: #9964

@pvorb
Copy link
Contributor Author

pvorb commented Aug 9, 2017

Thanks for mentioning. I haven't seen this one, yet. Please note, that I haven't been able to make it work in all three containers, yet. Undertow works out of the box, but Jetty and Tomcat are more involved and I'm still working on them.

@candrews
Copy link
Contributor

candrews commented Aug 9, 2017

For tomcat, you need to:

  • install tomcat-native (usually called libtcnative-1 as a package on Linux)
  • add the AprLifeCycleListener

Then your changes will work. (If you don't have tomcat native installed, tomcat will log an error and http2 won't work, but everything will otherwise still function)

@pvorb
Copy link
Contributor Author

pvorb commented Aug 9, 2017

Thanks for the hint, @candrews. I already saw that error message, when I tried my implementation. As I'm on Windows, I struggled to get it work using tomcat-native. I also tried to make it work using Java 9, but was unable to do so.

If you don't mind, I'd like to incorporate your solution using AprLifeCycleListener in my PR.

Then the only thing left to do would be Jetty support. I'll work on that later this evening.

@candrews
Copy link
Contributor

candrews commented Aug 9, 2017

If you don't mind, I'd like to incorporate your solution using AprLifeCycleListener in my PR.

Of course, please do :) FWIW, I think AprLifeCycleListener should be always added, regardless of the status of the new http2 setting.

@candrews
Copy link
Contributor

candrews commented Aug 9, 2017

Also, I think http2 should be enabled by default - why would it not be enabled by default?

@pvorb
Copy link
Contributor Author

pvorb commented Aug 9, 2017

Also, I think http2 should be enabled by default - why would it not be enabled by default?

I think when it works out of the box on all three Servlet Containers, we should consider enabling it by default. But until then, I think it's better to leave it disabled, i.e. I'd only enable it once Tomcat no longer needs tomcat-native to run it. Otherwise the error message might be misleading.

@candrews
Copy link
Contributor

candrews commented Aug 9, 2017

No server can support HTTP/2 on Java 8 without something like tomcat's tomcat-native for ALPN support.

I believe that enabling http2 by default and documenting that unless the selected server's native support library is installed or Java 9 is in use that HTTP/2 won't actually be enabled is the best way to go.

@pvorb
Copy link
Contributor Author

pvorb commented Aug 9, 2017

Maybe some of the Spring Boot main contributors (@wilkinsona, @philwebb, @snicoll, @dsyer) can share their thoughts on this topic?

@wilkinsona
Copy link
Member

No server can support HTTP/2 on Java 8 without something like tomcat's tomcat-native for ALPN support

Undertow's an exception to that. They use a "nasty hack" to avoid the need for modifying the bootstrap class path or using native code.

Otherwise the error message might be misleading

I think we definitely need to avoid a configuration that will lead to Tomcat logging an error (or warning) at startup.

I believe that enabling http2 by default and documenting that unless the selected server's native support library is installed or Java 9 is in use that HTTP/2 won't actually be enabled is the best way to go.

I could certainly be persuaded that enabling HTTP/2 by default is the way to go, but only if we can avoid the aforementioned error message from Tomcat. Perhaps we can automatically enable it with Tomcat but only if the necessary native component is available?

@pvorb
Copy link
Contributor Author

pvorb commented Aug 9, 2017

I could certainly be persuaded that enabling HTTP/2 by default is the way to go, but only if we can avoid the aforementioned error message from Tomcat. Perhaps we can automatically enable it with Tomcat but only if the necessary native component is available?

A tricky thing here would be to detect, whether a user explicitly wants to have HTTP/2 enabled on Tomcat – in that case you probably want to show the error from Tomcat that helps with properly configuring tomcat-native – or whether HTTP/2 is enabled by default. In that case you want to avoid the error message and silently fall back to HTTP/1.1.

@wilkinsona
Copy link
Member

A tricky thing here would be to detect, whether a user explicitly wants to have HTTP/2 enabled on Tomcat – in that case you probably want to show the error from Tomcat that helps with properly configuring tomcat-native – or whether HTTP/2 is enabled by default.

That's a good point. I haven't looked closely enough at how Tomcat finds the native library and the error it produces when it's not there to know for certain what our options are here. It feels like a general auto-configuration problem, though. A condition (that will then provide diagnostics in the auto-config report) could be one avenue that's worth exploring.

@pvorb
Copy link
Contributor Author

pvorb commented Aug 10, 2017

You can find the relevant code in the init() method of Tomcat's/Catalina's AprLifecycleListener. At least it's a starting point.

Thanks for your advice. I will try to find a solution after work.

@pvorb
Copy link
Contributor Author

pvorb commented Aug 10, 2017

Equipped with my notebook instead of my phone I found AprLifecycleListener.isAprAvailable(), which let's you easily detect tomcat-native. (Yes, navigating a code base can be hard on a phone.)

Unfortunately that method throws an IllegalAccessException on Java 9:

java.lang.IllegalAccessException: class org.apache.tomcat.util.buf.ByteBufferUtils cannot access class jdk.internal.ref.Cleaner (in module java.base) because module java.base does not export jdk.internal.ref to unnamed module @57fffcd7

😕

I'm still looking for another solution...

@pvorb
Copy link
Contributor Author

pvorb commented Aug 10, 2017

Jetty relies on modifying the bootclasspath. I'm not sure whether to require Spring Boot users to do that when running their app is a viable option.

Two solutions come to my mind:

  1. Decide that HTTP/2 will only be supported for Java 9 and later.
  2. Provide our own nasty hack to support ALPN on Java 8

What do you think? Is there a general solution to problems like this one, where a feature is not equally supported by all three Servlet Containers?

@pvorb
Copy link
Contributor Author

pvorb commented Aug 11, 2017

Maybe better than writing our own hack would be to use netty-tcnative-boringssl-static, which bundles tomcat-native binaries for common platforms. But I also don't really like this solution as it would make spring-boot fat jars a lot fatter.

So I suggest enabling HTTP/2 only on Java 9 by default would be a sane solution. On Java 8 we could require users to provide the container specific solution to the ALPN problem themselves, if they want to enable HTTP/2.

@wilkinsona
Copy link
Member

I don't think we should write our own nasty hack. Enabling HTTP/2 by default only on Java 9 seems like a good compromise to me. Particularly if we can also make things a bit easier for those on Java 8 as well (that could just be documentation, or documentation and some simplified configuration).

@pvorb pvorb changed the base branch from master to jdk9 August 13, 2017 18:04
@snicoll snicoll force-pushed the jdk9 branch 2 times, most recently from 143f2eb to 02ed20d Compare August 16, 2017 15:54
@snicoll
Copy link
Member

snicoll commented Aug 16, 2017

Requiring Java9 for the whole build makes this PR unlikely to be merged. Do you need Java9 at runtime or are you compiling against Java 9 APIs? In the case of the latter, we need to go reflection route. As for the former, we need something that prevents the test to run when Java9 is not present, see #9530

@pvorb
Copy link
Contributor Author

pvorb commented Aug 16, 2017

@snicoll Thanks for the hint. I just rebased this feature on the branch jdk9, which already configures Travis CI to run using oraclejdk9. Some of the changes that enable spring-boot to work on Java 9 are required to test this PR's functionality, so I thought rebasing on jdk9 would be the way to go.

I can mark the test, which requires Java 9, as @Ignored, but then the CI build would still run against Java 9, wouldn't it? Probably I'm missing something...

Do you have a clue on how to proceed?

@pvorb pvorb changed the base branch from jdk9 to master August 16, 2017 19:43
@pvorb
Copy link
Contributor Author

pvorb commented Aug 20, 2017

It turned out that it was a little harder to get Jetty's HTTP/2 support to work on Java 9. It took some time until I found the brand new

<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-alpn-java-server</artifactId>
<version>9.4.7.RC0</version>

which provides ALPN support for Jetty using Java 9. As this is a first Release Candidate for Jetty 9.4.7, it might make sense to wait for the first Jetty 9.4.7 stable release before this will be supported out of the box by Spring Boot.

Here's the thread where I found out about the module: jetty/jetty.project#486

@pvorb
Copy link
Contributor Author

pvorb commented Sep 20, 2017

Since Jetty 9.4.7 has been released on 2017-09-14, I'll continue working on this branch.

@pvorb
Copy link
Contributor Author

pvorb commented Sep 28, 2017

I've configured jetty-alpn-java-server in spring-boot-dependencies and I've set jetty.version to 9.4.7.v20170914. My next goal is removing he server.http2.enabled property and configuring HTTP/2 directly in the *ServletWebServerFactories, i.e. TomcatServletWebServerFactory, JettyServletWebServerFactory and UndertowServletWebServerFactory. Once I have that working, WebFlux/Netty will come next.

@bclozel bclozel self-assigned this Sep 29, 2017
@bclozel
Copy link
Member

bclozel commented Sep 29, 2017

Could we take a step back and stop adding things in that PR?
More commits just decreases the chances of getting this merged.

Let me come up with a detailed proposal in #10043 first and then we can cherry-pick things from this PR.

@bclozel
Copy link
Member

bclozel commented Nov 3, 2017

Hey @pvorb - thanks for your efforts.
As you've probably seen this has been partially applied in #10043 - but Jetty support is scheduled for #10902.
I'm leaving this PR opened as I'd like to cherry pick things from here for Jetty.

@pvorb
Copy link
Contributor Author

pvorb commented Nov 3, 2017

Awesome to hear that you're making progress.

@pvorb pvorb changed the title [WIP] Common way to configure HTTP/2 Common way to configure HTTP/2 Nov 23, 2017
@bclozel
Copy link
Member

bclozel commented Jan 3, 2018

Cherry-picking was mostly done from #10041

@bclozel bclozel closed this Jan 3, 2018
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged label Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants