Thank you Jacques and Taher for your review and useful comments: I have committed this work with rev. 1854532.
Taher, I have reviewed the configuration options but I don't feel like changing any of them by default; for now I have added a comment to the commit about them and I will leave the implementation of the ability to set them to a future enhancement. As a side note, this is a good example for a valid reason to provide the ability to run OFBiz in an external (rather than embedded) Tomcat instance (for which all the configuration options would be available outside of OFBiz): this would require to refactor various aspects of our framework, so I will postpone this discussion to another day :-) Regards, Jacopo On Mon, Feb 25, 2019 at 10:48 AM Jacques Le Roux < jacques.le.r...@les7arts.com> wrote: > I had a look, I don't think we need to worry about those vars. > > If/When needed we can change defaults. > > Jacques > > Le 19/02/2019 à 19:05, Jacques Le Roux a écrit : > > Good question, I guess most are OK, but what are Trailer Headers for > instance? > > > > BTW https://tomcat.apache.org/tomcat-9.0-doc/config/http2.html is our > reference for the trunk > > > > Jacques > > > > Le 19/02/2019 à 10:24, Taher Alkhateeb a écrit : > >> clean and simple implementation, +1 > >> > >> on a side note, I wonder if we need to set any of the http2 attributes > >> listed in [1] or whether the defaults are okay. > >> > >> [1] https://tomcat.apache.org/tomcat-8.5-doc/config/http2.html > >> > >> On Mon, Feb 18, 2019 at 5:58 PM Jacques Le Roux > >> <jacques.le.r...@les7arts.com> wrote: > >>> Hi Jacopo, > >>> > >>> Sounds good to me, we can always easily revert in case of unexpected > issue anyway > >>> > >>> Thanks > >>> > >>> Jacques > >>> > >>> Le 18/02/2019 à 11:43, Jacopo Cappellato a écrit : > >>>> Hi all, > >>>> > >>>> I think it is time to enable the instance of Tomcat that is embedded > in > >>>> OFBiz to communicate using the HTTP/2 protocol, when the client > supports it. > >>>> For your review, before I commit, I am pasting here the patch that > will > >>>> enable it (it is quite simple) . > >>>> In it I have enabled HTTP/2 by default, by setting > upgradeProtocol=true, in > >>>> the http and https connectors (but they will continue to support also > >>>> HTTP/1.1); if the new property "upgradeProtocol", that I have > introduced > >>>> for this specific purpose, is not set (as it would be the case in > custom > >>>> configuration files) then the new protocol will not be enabled. Does > the > >>>> approach look good to you? > >>>> > >>>> Thanks, > >>>> > >>>> Jacopo > >>>> > >>>> PS: you can test it, for example, using curl: > >>>> > >>>> curl -vso /dev/null --http2 http://localhost:8080 > >>>> > >>>> > >>>> Index: framework/catalina/ofbiz-component.xml > >>>> =================================================================== > >>>> --- framework/catalina/ofbiz-component.xml (revision 1853787) > >>>> +++ framework/catalina/ofbiz-component.xml (working copy) > >>>> @@ -99,6 +99,7 @@ > >>>> <!--<property name="address" value=""/>--> > >>>> <property name="port" value="8080"/> > >>>> <property name="protocol" value="HTTP/1.1"/> > >>>> + <property name="upgradeProtocol" value="true"/> > >>>> <property name="scheme" value="http"/> > >>>> <property name="secure" value="false"/> > >>>> <property name="URIEncoding" value="UTF-8"/> > >>>> @@ -128,6 +129,7 @@ > >>>> <!--<property name="address" value=""/>--> > >>>> <property name="port" value="8443"/> > >>>> <property name="protocol" value="HTTP/1.1"/> > >>>> + <property name="upgradeProtocol" value="true"/> > >>>> <property name="scheme" value="https"/> > >>>> <property name="secure" value="true"/> > >>>> <property name="SSLEnabled" value="true"/> > >>>> @@ -183,6 +185,7 @@ > >>>> <!--<property name="address" value=""/>--> > >>>> <property name="port" value="8080"/> > >>>> <property name="protocol" value="HTTP/1.1"/> > >>>> + <property name="upgradeProtocol" value="true"/> > >>>> <property name="scheme" value="http"/> > >>>> <property name="secure" value="false"/> > >>>> <property name="URIEncoding" value="UTF-8"/> > >>>> @@ -194,6 +197,7 @@ > >>>> <!--<property name="address" value=""/>--> > >>>> <property name="port" value="8443"/> > >>>> <property name="protocol" value="HTTP/1.1"/> > >>>> + <property name="upgradeProtocol" value="true"/> > >>>> <property name="scheme" value="https"/> > >>>> <property name="secure" value="true"/> > >>>> <property name="SSLEnabled" value="true"/> > >>>> Index: > >>>> > framework/catalina/src/main/java/org/apache/ofbiz/catalina/container/CatalinaContainer.java > >>>> =================================================================== > >>>> --- > >>>> > framework/catalina/src/main/java/org/apache/ofbiz/catalina/container/CatalinaContainer.java > >>>> (revision > >>>> 1853787) > >>>> +++ > >>>> > framework/catalina/src/main/java/org/apache/ofbiz/catalina/container/CatalinaContainer.java > >>>> (working > >>>> copy) > >>>> @@ -63,6 +63,7 @@ > >>>> import org.apache.catalina.util.ServerInfo; > >>>> import org.apache.catalina.valves.AccessLogValve; > >>>> import org.apache.catalina.webresources.StandardRoot; > >>>> +import org.apache.coyote.http2.Http2Protocol; > >>>> import org.apache.ofbiz.base.component.ComponentConfig; > >>>> import org.apache.ofbiz.base.concurrent.ExecutionPool; > >>>> import org.apache.ofbiz.base.container.Container; > >>>> @@ -417,9 +418,12 @@ > >>>> private Connector prepareConnector(Property connectorProp) { > >>>> Connector connector = new > >>>> Connector(ContainerConfig.getPropertyValue(connectorProp, "protocol", > >>>> "HTTP/1.1")); > >>>> connector.setPort(ContainerConfig.getPropertyValue(connectorProp, > >>>> "port", 0) + Start.getInstance().getConfig().portOffset); > >>>> - > >>>> + if > ("true".equals(ContainerConfig.getPropertyValue(connectorProp, > >>>> "upgradeProtocol", "false"))) { > >>>> + connector.addUpgradeProtocol(new Http2Protocol()); > >>>> + Debug.logInfo("Tomcat " + connector + ": enabled HTTP/2", > >>>> module); > >>>> + } > >>>> connectorProp.properties.values().stream() > >>>> - .filter(prop -> !"protocol".equals(prop.name) && > >>>> !"port".equals(prop.name)) > >>>> + .filter(prop -> !"protocol".equals(prop.name) && > >>>> !"upgradeProtocol".equals(prop.name) && !"port".equals(prop.name)) > >>>> .forEach(prop -> { > >>>> if (IntrospectionUtils.setProperty(connector, > prop.name, > >>>> prop.value)) { > >>>> if (prop.name.indexOf("Pass") != -1) { > >>>> > > >
