You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Filip Hanik <fh...@pivotal.io> on 2020/04/06 16:56:08 UTC
API Change - Connector.java Constructor
Team,
As I'm slowly transitioning between projects, Apache Tomcat has once again
showed up in my workspace. I'm currently working on improving the embedded
experience for native images.
I have a pull request, [1] <https://github.com/apache/tomcat/pull/267>,
that I'd like to open up a discussion about
Goal: Able to start embedded Apache Tomcat without using reflection.
This PR: Changes the constructor for Connector to not use reflection.
Instead the calling components will instantiate the ProtocolHandler
Besides the constructor change, functionality should remain backwards
compatible.
Please provide feedback, such as
1. Are we open to making API changes like this?
2. Does the PR introduce any problems?
Thank you
Filip
[1] https://github.com/apache/tomcat/pull/267
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Apr 7, 2020 at 10:16 AM Mark Thomas <ma...@apache.org> wrote:
> My thinking was more along the following lines...
>
> Rewrite the Connector (and possibly some/all other components) so that
>
If we're talking about embedded, then the other components don't have the
problem. For example to create a context you can do new Context and add it
to a Host, there's no reflection. That's my basic understanding at least.
Personally, I dislike true embedded in favor of using server.xml and in
that case it's all reflection. Although the binary is "large" I haven't
noticed this was a problem.
> if a known class name was specified, rather than using the specified
> name directly via reflection we could do something like:
>
> if ("org.apache.coyote.http11.Http11NioProtocol".equals(className)) {
> protocol = new Http11NioProtocol();
> } else if ("org...
> ...
> } else {
> // OK. Unrecognised class name. Use reflection
> ...
> }
>
> Would that style of approach help at all?
>
I like it, so I'll work on that. I'll also clear up the AJP hack.
Rémy
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: API Change - Connector.java Constructor
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Doesn't the same applies? It is a finite product (and not too big). The
only choice to do is how to expose it I guess.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le jeu. 16 avr. 2020 à 14:34, Rémy Maucherat <re...@apache.org> a écrit :
> On Thu, Apr 16, 2020 at 1:42 PM Romain Manni-Bucau <rm...@gmail.com>
> wrote:
>
>> Hi all,
>>
>> Just wanted to shout a proposal - hoping to not be too much "off".
>>
>> I guess the target of bypassing reflection is mainly for what tomcat
>> delivers - ie not the user/vendor extensions - so it means everything is
>> there at build time, so why not generating the reflection-free API?
>> Idea would be the following one: for each class "missing" setters (to
>> bypass reflection), generate a <classname>$Accessors class extending the
>> root one but with the accessors (the nested class enables to have access to
>> private fields).
>> Small trick: some $Accessors class must have multiple flavors, I'm
>> thinking to transitive reflection (protocol handler) but since all the
>> settable model is known at build time it is very doable.
>> Simplest sounds using a first compilation pass, generation then compile
>> new classes but using an annotation processor sounds doable as well.
>>
>> At the end, this means that then you can be fully reflection free using
>> the generated classes instead of the standard one.
>>
>> Hope it makes sense and it is not too late.
>>
>
> The get/set are methods that could/would be removed of course, but I
> haven't even done it at all right now. The problem is the object creation
> and how and when they get tied together.
>
> Rémy
>
>
>>
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>> <https://rmannibucau.metawerx.net/> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github
>> <https://github.com/rmannibucau> | LinkedIn
>> <https://www.linkedin.com/in/rmannibucau> | Book
>> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>
>>
>> Le mer. 15 avr. 2020 à 18:26, Rémy Maucherat <re...@apache.org> a écrit :
>>
>>> On Fri, Apr 10, 2020 at 6:32 PM Filip Hanik <fh...@pivotal.io> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Apr 10, 2020 at 1:28 AM Rémy Maucherat <re...@apache.org> wrote:
>>>>
>>>>>
>>>>>
>>>>>> This configuration gives the impression that the Endpoint is a child
>>>>>> of the Connector.
>>>>>> But the Connector truly only needs the ProtocolHandler interface to
>>>>>> function. The injected object would then be better to an instance of a
>>>>>> ProtocolHandler
>>>>>>
>>>>>> The XML can of course be configured to instantiate and inject the
>>>>>> ProtocolHandler handler directly into the Connector
>>>>>> In this setting, it doesn't make sense to have any properties on the
>>>>>> Connector, since the Connector receives the protocol handler already
>>>>>> configured.
>>>>>>
>>>>>> <Connector scheme="https" secure="true">
>>>>>> <Protocol className="org.apache.coyote.http11.Http11Protocol"
>>>>>> maxHeaderCount="10" >
>>>>>> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
>>>>>> port="8443" SSLEnabled="true"
>>>>>>
>>>>>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>>>>>> <SocketProperties directBuffer="true"
>>>>>> directSslBuffer="true" />
>>>>>> <SSLHostConfig honorCipherOrder="false">
>>>>>> <Certificate
>>>>>> certificateKeyFile="${catalina.home}/conf/key.pem"
>>>>>>
>>>>>> certificateFile="${catalina.home}/conf/cert.pem"
>>>>>> type="RSA" />
>>>>>> </SSLHostConfig>
>>>>>> </Endpoint>
>>>>>> <UpgradeProtocol
>>>>>> className="org.apache.coyote.http2.Http2Protocol" />
>>>>>> <Protocol
>>>>>> </Connector>
>>>>>>
>>>>>
>>>>> Either way, I experimented a bit and it's not doable. Too many
>>>>> intrusive changes and impossibility to be compatible.
>>>>>
>>>>
>>>> Sounds good.
>>>>
>>>
>>> I started working on it more to make a real attempt and see how it
>>> behaves in practice. Even though the changes are problematic [the biggest
>>> Catalina/Tomcat API break ever, surpassing the TLS configuration changes
>>> earlier], the Connector is still the biggest problem for duplicated
>>> properties and random hacks, including reflection abuse. That's a goal/todo
>>> for 10 so it is worth doing it to put it on review to know if it exceeds
>>> the pain threshold of most.
>>>
>>> Rémy
>>>
>>>
>>>>
>>>> Filip
>>>>
>>>
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Apr 16, 2020 at 1:42 PM Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Hi all,
>
> Just wanted to shout a proposal - hoping to not be too much "off".
>
> I guess the target of bypassing reflection is mainly for what tomcat
> delivers - ie not the user/vendor extensions - so it means everything is
> there at build time, so why not generating the reflection-free API?
> Idea would be the following one: for each class "missing" setters (to
> bypass reflection), generate a <classname>$Accessors class extending the
> root one but with the accessors (the nested class enables to have access to
> private fields).
> Small trick: some $Accessors class must have multiple flavors, I'm
> thinking to transitive reflection (protocol handler) but since all the
> settable model is known at build time it is very doable.
> Simplest sounds using a first compilation pass, generation then compile
> new classes but using an annotation processor sounds doable as well.
>
> At the end, this means that then you can be fully reflection free using
> the generated classes instead of the standard one.
>
> Hope it makes sense and it is not too late.
>
The get/set are methods that could/would be removed of course, but I
haven't even done it at all right now. The problem is the object creation
and how and when they get tied together.
Rémy
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le mer. 15 avr. 2020 à 18:26, Rémy Maucherat <re...@apache.org> a écrit :
>
>> On Fri, Apr 10, 2020 at 6:32 PM Filip Hanik <fh...@pivotal.io> wrote:
>>
>>>
>>>
>>> On Fri, Apr 10, 2020 at 1:28 AM Rémy Maucherat <re...@apache.org> wrote:
>>>
>>>>
>>>>
>>>>> This configuration gives the impression that the Endpoint is a child
>>>>> of the Connector.
>>>>> But the Connector truly only needs the ProtocolHandler interface to
>>>>> function. The injected object would then be better to an instance of a
>>>>> ProtocolHandler
>>>>>
>>>>> The XML can of course be configured to instantiate and inject the
>>>>> ProtocolHandler handler directly into the Connector
>>>>> In this setting, it doesn't make sense to have any properties on the
>>>>> Connector, since the Connector receives the protocol handler already
>>>>> configured.
>>>>>
>>>>> <Connector scheme="https" secure="true">
>>>>> <Protocol className="org.apache.coyote.http11.Http11Protocol"
>>>>> maxHeaderCount="10" >
>>>>> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
>>>>> port="8443" SSLEnabled="true"
>>>>>
>>>>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>>>>> <SocketProperties directBuffer="true" directSslBuffer="true"
>>>>> />
>>>>> <SSLHostConfig honorCipherOrder="false">
>>>>> <Certificate
>>>>> certificateKeyFile="${catalina.home}/conf/key.pem"
>>>>>
>>>>> certificateFile="${catalina.home}/conf/cert.pem"
>>>>> type="RSA" />
>>>>> </SSLHostConfig>
>>>>> </Endpoint>
>>>>> <UpgradeProtocol
>>>>> className="org.apache.coyote.http2.Http2Protocol" />
>>>>> <Protocol
>>>>> </Connector>
>>>>>
>>>>
>>>> Either way, I experimented a bit and it's not doable. Too many
>>>> intrusive changes and impossibility to be compatible.
>>>>
>>>
>>> Sounds good.
>>>
>>
>> I started working on it more to make a real attempt and see how it
>> behaves in practice. Even though the changes are problematic [the biggest
>> Catalina/Tomcat API break ever, surpassing the TLS configuration changes
>> earlier], the Connector is still the biggest problem for duplicated
>> properties and random hacks, including reflection abuse. That's a goal/todo
>> for 10 so it is worth doing it to put it on review to know if it exceeds
>> the pain threshold of most.
>>
>> Rémy
>>
>>
>>>
>>> Filip
>>>
>>
Re: API Change - Connector.java Constructor
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi all,
Just wanted to shout a proposal - hoping to not be too much "off".
I guess the target of bypassing reflection is mainly for what tomcat
delivers - ie not the user/vendor extensions - so it means everything is
there at build time, so why not generating the reflection-free API?
Idea would be the following one: for each class "missing" setters (to
bypass reflection), generate a <classname>$Accessors class extending the
root one but with the accessors (the nested class enables to have access to
private fields).
Small trick: some $Accessors class must have multiple flavors, I'm thinking
to transitive reflection (protocol handler) but since all the settable
model is known at build time it is very doable.
Simplest sounds using a first compilation pass, generation then compile new
classes but using an annotation processor sounds doable as well.
At the end, this means that then you can be fully reflection free using the
generated classes instead of the standard one.
Hope it makes sense and it is not too late.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mer. 15 avr. 2020 à 18:26, Rémy Maucherat <re...@apache.org> a écrit :
> On Fri, Apr 10, 2020 at 6:32 PM Filip Hanik <fh...@pivotal.io> wrote:
>
>>
>>
>> On Fri, Apr 10, 2020 at 1:28 AM Rémy Maucherat <re...@apache.org> wrote:
>>
>>>
>>>
>>>> This configuration gives the impression that the Endpoint is a child of
>>>> the Connector.
>>>> But the Connector truly only needs the ProtocolHandler interface to
>>>> function. The injected object would then be better to an instance of a
>>>> ProtocolHandler
>>>>
>>>> The XML can of course be configured to instantiate and inject the
>>>> ProtocolHandler handler directly into the Connector
>>>> In this setting, it doesn't make sense to have any properties on the
>>>> Connector, since the Connector receives the protocol handler already
>>>> configured.
>>>>
>>>> <Connector scheme="https" secure="true">
>>>> <Protocol className="org.apache.coyote.http11.Http11Protocol"
>>>> maxHeaderCount="10" >
>>>> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
>>>> port="8443" SSLEnabled="true"
>>>>
>>>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>>>> <SocketProperties directBuffer="true" directSslBuffer="true"
>>>> />
>>>> <SSLHostConfig honorCipherOrder="false">
>>>> <Certificate
>>>> certificateKeyFile="${catalina.home}/conf/key.pem"
>>>>
>>>> certificateFile="${catalina.home}/conf/cert.pem"
>>>> type="RSA" />
>>>> </SSLHostConfig>
>>>> </Endpoint>
>>>> <UpgradeProtocol
>>>> className="org.apache.coyote.http2.Http2Protocol" />
>>>> <Protocol
>>>> </Connector>
>>>>
>>>
>>> Either way, I experimented a bit and it's not doable. Too many intrusive
>>> changes and impossibility to be compatible.
>>>
>>
>> Sounds good.
>>
>
> I started working on it more to make a real attempt and see how it behaves
> in practice. Even though the changes are problematic [the biggest
> Catalina/Tomcat API break ever, surpassing the TLS configuration changes
> earlier], the Connector is still the biggest problem for duplicated
> properties and random hacks, including reflection abuse. That's a goal/todo
> for 10 so it is worth doing it to put it on review to know if it exceeds
> the pain threshold of most.
>
> Rémy
>
>
>>
>> Filip
>>
>
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Fri, Apr 10, 2020 at 6:32 PM Filip Hanik <fh...@pivotal.io> wrote:
>
>
> On Fri, Apr 10, 2020 at 1:28 AM Rémy Maucherat <re...@apache.org> wrote:
>
>>
>>
>>> This configuration gives the impression that the Endpoint is a child of
>>> the Connector.
>>> But the Connector truly only needs the ProtocolHandler interface to
>>> function. The injected object would then be better to an instance of a
>>> ProtocolHandler
>>>
>>> The XML can of course be configured to instantiate and inject the
>>> ProtocolHandler handler directly into the Connector
>>> In this setting, it doesn't make sense to have any properties on the
>>> Connector, since the Connector receives the protocol handler already
>>> configured.
>>>
>>> <Connector scheme="https" secure="true">
>>> <Protocol className="org.apache.coyote.http11.Http11Protocol"
>>> maxHeaderCount="10" >
>>> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
>>> port="8443" SSLEnabled="true"
>>>
>>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>>> <SocketProperties directBuffer="true" directSslBuffer="true" />
>>> <SSLHostConfig honorCipherOrder="false">
>>> <Certificate
>>> certificateKeyFile="${catalina.home}/conf/key.pem"
>>> certificateFile="${catalina.home}/conf/cert.pem"
>>> type="RSA" />
>>> </SSLHostConfig>
>>> </Endpoint>
>>> <UpgradeProtocol
>>> className="org.apache.coyote.http2.Http2Protocol" />
>>> <Protocol
>>> </Connector>
>>>
>>
>> Either way, I experimented a bit and it's not doable. Too many intrusive
>> changes and impossibility to be compatible.
>>
>
> Sounds good.
>
I started working on it more to make a real attempt and see how it behaves
in practice. Even though the changes are problematic [the biggest
Catalina/Tomcat API break ever, surpassing the TLS configuration changes
earlier], the Connector is still the biggest problem for duplicated
properties and random hacks, including reflection abuse. That's a goal/todo
for 10 so it is worth doing it to put it on review to know if it exceeds
the pain threshold of most.
Rémy
>
> Filip
>
Re: API Change - Connector.java Constructor
Posted by Filip Hanik <fh...@pivotal.io>.
On Fri, Apr 10, 2020 at 1:28 AM Rémy Maucherat <re...@apache.org> wrote:
>
>
>> This configuration gives the impression that the Endpoint is a child of
>> the Connector.
>> But the Connector truly only needs the ProtocolHandler interface to
>> function. The injected object would then be better to an instance of a
>> ProtocolHandler
>>
>> The XML can of course be configured to instantiate and inject the
>> ProtocolHandler handler directly into the Connector
>> In this setting, it doesn't make sense to have any properties on the
>> Connector, since the Connector receives the protocol handler already
>> configured.
>>
>> <Connector scheme="https" secure="true">
>> <Protocol className="org.apache.coyote.http11.Http11Protocol"
>> maxHeaderCount="10" >
>> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
>> port="8443" SSLEnabled="true"
>>
>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>> <SocketProperties directBuffer="true" directSslBuffer="true" />
>> <SSLHostConfig honorCipherOrder="false">
>> <Certificate
>> certificateKeyFile="${catalina.home}/conf/key.pem"
>> certificateFile="${catalina.home}/conf/cert.pem"
>> type="RSA" />
>> </SSLHostConfig>
>> </Endpoint>
>> <UpgradeProtocol
>> className="org.apache.coyote.http2.Http2Protocol" />
>> <Protocol
>> </Connector>
>>
>
> Either way, I experimented a bit and it's not doable. Too many intrusive
> changes and impossibility to be compatible.
>
Sounds good.
Filip
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Apr 9, 2020 at 7:54 PM Filip Hanik <fh...@pivotal.io> wrote:
> Thanks Remy,
>
> On Wed, Apr 8, 2020 at 8:48 AM Rémy Maucherat <re...@apache.org> wrote:
>
>>
>>>
>> If we want to improve on the Connector situation regarding duplication
>> and reflection abuse, the only solution is to expose the different objects
>> involved.
>>
>> Since an example is usually better, I'll give one using server.xml.
>>
>> A typical Connector with TLS is at the moment:
>> <Connector port="8443"
>> protocol="org.apache.coyote.http11.Http11NioProtocol"
>> SSLEnabled="true" scheme="https" secure="true"
>> socket.directBuffer="true" socket.directSslBuffer="true"
>> maxHeaderCount="10"
>>
>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>> <SSLHostConfig honorCipherOrder="false">
>> <Certificate
>> certificateKeyFile="${catalina.home}/conf/key.pem"
>> certificateFile="${catalina.home}/conf/cert.pem"
>> type="RSA" />
>> </SSLHostConfig>
>> <UpgradeProtocol
>> className="org.apache.coyote.http2.Http2Protocol" />
>> </Connector>
>>
>> And it would become:
>> <Connector scheme="https" secure="true">
>> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
>> port="8443" SSLEnabled="true"
>>
>> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
>> <SocketProperties directBuffer="true" directSslBuffer="true" />
>> <SSLHostConfig honorCipherOrder="false">
>> <Certificate
>> certificateKeyFile="${catalina.home}/conf/key.pem"
>> certificateFile="${catalina.home}/conf/cert.pem"
>> type="RSA" />
>> </SSLHostConfig>
>> </Endpoint>
>> <Protocol className="org.apache.coyote.http11.Http11Protocol"
>> maxHeaderCount="10" />
>> <UpgradeProtocol
>> className="org.apache.coyote.http2.Http2Protocol" />
>> </Connector>
>>
>
> This configuration gives the impression that the Endpoint is a child of
> the Connector.
> But the Connector truly only needs the ProtocolHandler interface to
> function. The injected object would then be better to an instance of a
> ProtocolHandler
>
> The XML can of course be configured to instantiate and inject the
> ProtocolHandler handler directly into the Connector
> In this setting, it doesn't make sense to have any properties on the
> Connector, since the Connector receives the protocol handler already
> configured.
>
> <Connector scheme="https" secure="true">
> <Protocol className="org.apache.coyote.http11.Http11Protocol"
> maxHeaderCount="10" >
> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
> port="8443" SSLEnabled="true"
>
> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
> <SocketProperties directBuffer="true" directSslBuffer="true" />
> <SSLHostConfig honorCipherOrder="false">
> <Certificate certificateKeyFile="${catalina.home}/conf/key.pem"
> certificateFile="${catalina.home}/conf/cert.pem"
> type="RSA" />
> </SSLHostConfig>
> </Endpoint>
> <UpgradeProtocol
> className="org.apache.coyote.http2.Http2Protocol" />
> <Protocol
> </Connector>
>
Either way, I experimented a bit and it's not doable. Too many intrusive
changes and impossibility to be compatible.
Rémy
>
> Filip
>
>
>> t
>> Each individual object is now created by the digester using normal bean
>> rules, and I suppose it will become wired up by the Connector during init.
>> Embedded can then do the same as the digester instead of having to go
>> through the Connector object.
>>
>> Rémy
>>
>>
>>>
>>> Mark
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>>
Re: API Change - Connector.java Constructor
Posted by Filip Hanik <fh...@pivotal.io>.
Thanks Remy,
On Wed, Apr 8, 2020 at 8:48 AM Rémy Maucherat <re...@apache.org> wrote:
>
>>
> If we want to improve on the Connector situation regarding duplication and
> reflection abuse, the only solution is to expose the different objects
> involved.
>
> Since an example is usually better, I'll give one using server.xml.
>
> A typical Connector with TLS is at the moment:
> <Connector port="8443"
> protocol="org.apache.coyote.http11.Http11NioProtocol"
> SSLEnabled="true" scheme="https" secure="true"
> socket.directBuffer="true" socket.directSslBuffer="true"
> maxHeaderCount="10"
>
> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
> <SSLHostConfig honorCipherOrder="false">
> <Certificate certificateKeyFile="${catalina.home}/conf/key.pem"
> certificateFile="${catalina.home}/conf/cert.pem"
> type="RSA" />
> </SSLHostConfig>
> <UpgradeProtocol className="org.apache.coyote.http2.Http2Protocol"
> />
> </Connector>
>
> And it would become:
> <Connector scheme="https" secure="true">
> <Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
> port="8443" SSLEnabled="true"
>
> sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
> <SocketProperties directBuffer="true" directSslBuffer="true" />
> <SSLHostConfig honorCipherOrder="false">
> <Certificate certificateKeyFile="${catalina.home}/conf/key.pem"
> certificateFile="${catalina.home}/conf/cert.pem"
> type="RSA" />
> </SSLHostConfig>
> </Endpoint>
> <Protocol className="org.apache.coyote.http11.Http11Protocol"
> maxHeaderCount="10" />
> <UpgradeProtocol className="org.apache.coyote.http2.Http2Protocol"
> />
> </Connector>
>
This configuration gives the impression that the Endpoint is a child of the
Connector.
But the Connector truly only needs the ProtocolHandler interface to
function. The injected object would then be better to an instance of a
ProtocolHandler
The XML can of course be configured to instantiate and inject the
ProtocolHandler handler directly into the Connector
In this setting, it doesn't make sense to have any properties on the
Connector, since the Connector receives the protocol handler already
configured.
<Connector scheme="https" secure="true">
<Protocol className="org.apache.coyote.http11.Http11Protocol"
maxHeaderCount="10" >
<Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
port="8443" SSLEnabled="true"
sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
<SocketProperties directBuffer="true" directSslBuffer="true" />
<SSLHostConfig honorCipherOrder="false">
<Certificate certificateKeyFile="${catalina.home}/conf/key.pem"
certificateFile="${catalina.home}/conf/cert.pem"
type="RSA" />
</SSLHostConfig>
</Endpoint>
<UpgradeProtocol
className="org.apache.coyote.http2.Http2Protocol" />
<Protocol
</Connector>
Filip
> t
> Each individual object is now created by the digester using normal bean
> rules, and I suppose it will become wired up by the Connector during init.
> Embedded can then do the same as the digester instead of having to go
> through the Connector object.
>
> Rémy
>
>
>>
>> Mark
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Apr 7, 2020 at 8:42 PM Mark Thomas <ma...@apache.org> wrote:
> On 07/04/2020 19:03, Filip Hanik wrote:
> >
> >
> > On Tue, Apr 7, 2020 at 9:35 AM Rémy Maucherat <remm@apache.org
> > <ma...@apache.org>> wrote:
> >
> >
> > Does the connector need to know about the actual implementations?
> >
> >
> > Ideally no, but it removes the reflection you say is bad for Graal.
> >
> >
> > Correct. Turns out that the connectors use setProperty/getProperty via
> > reflection (IntrospectionUtils.setProperty/getProperty), so changing
> > only this constructor would achieve a mini step.
> > Before we commit any changes, I'd like to evaluate the scope of
> > reflection we're dealing with.
> >
> > Then I can come back. I'll close the PR for now, as it only touches the
> > surface.
> >
> > sound fair?
>
> Sounds reasonable. I'm happy for any obvious clean-up to stay though.
>
> On a similar note, the ProtocolHandler calls setProperty() on the
> Endpoint which then also uses reflection.
>
> I think I have a way around this but it is not great for maintenance.
>
If we want to improve on the Connector situation regarding duplication and
reflection abuse, the only solution is to expose the different objects
involved.
Since an example is usually better, I'll give one using server.xml.
A typical Connector with TLS is at the moment:
<Connector port="8443"
protocol="org.apache.coyote.http11.Http11NioProtocol"
SSLEnabled="true" scheme="https" secure="true"
socket.directBuffer="true" socket.directSslBuffer="true"
maxHeaderCount="10"
sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
<SSLHostConfig honorCipherOrder="false">
<Certificate certificateKeyFile="${catalina.home}/conf/key.pem"
certificateFile="${catalina.home}/conf/cert.pem"
type="RSA" />
</SSLHostConfig>
<UpgradeProtocol className="org.apache.coyote.http2.Http2Protocol"
/>
</Connector>
And it would become:
<Connector scheme="https" secure="true">
<Endpoint className="org.apache.tomcat.util.net.NioEndpoint"
port="8443" SSLEnabled="true"
sslImplementationName="org.apache.tomcat.util.net.openssl.OpenSSLImplementation">
<SocketProperties directBuffer="true" directSslBuffer="true" />
<SSLHostConfig honorCipherOrder="false">
<Certificate certificateKeyFile="${catalina.home}/conf/key.pem"
certificateFile="${catalina.home}/conf/cert.pem"
type="RSA" />
</SSLHostConfig>
</Endpoint>
<Protocol className="org.apache.coyote.http11.Http11Protocol"
maxHeaderCount="10" />
<UpgradeProtocol className="org.apache.coyote.http2.Http2Protocol"
/>
</Connector>
Each individual object is now created by the digester using normal bean
rules, and I suppose it will become wired up by the Connector during init.
Embedded can then do the same as the digester instead of having to go
through the Connector object.
Rémy
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Apr 7, 2020 at 8:42 PM Mark Thomas <ma...@apache.org> wrote:
> On 07/04/2020 19:03, Filip Hanik wrote:
> >
> >
> > On Tue, Apr 7, 2020 at 9:35 AM Rémy Maucherat <remm@apache.org
> > <ma...@apache.org>> wrote:
> >
> >
> > Does the connector need to know about the actual implementations?
> >
> >
> > Ideally no, but it removes the reflection you say is bad for Graal.
> >
> >
> > Correct. Turns out that the connectors use setProperty/getProperty via
> > reflection (IntrospectionUtils.setProperty/getProperty), so changing
> > only this constructor would achieve a mini step.
> > Before we commit any changes, I'd like to evaluate the scope of
> > reflection we're dealing with.
> >
> > Then I can come back. I'll close the PR for now, as it only touches the
> > surface.
> >
> > sound fair?
>
> Sounds reasonable. I'm happy for any obvious clean-up to stay though.
>
> On a similar note, the ProtocolHandler calls setProperty() on the
> Endpoint which then also uses reflection.
>
> I think I have a way around this but it is not great for maintenance.
>
Yes, I don't see how it could be a good move.
The Connector creation cleanup sounded like a good refactoring however,
especially the [horrible] AJP part of it.
Rémy
Re: API Change - Connector.java Constructor
Posted by Mark Thomas <ma...@apache.org>.
On 07/04/2020 19:03, Filip Hanik wrote:
>
>
> On Tue, Apr 7, 2020 at 9:35 AM Rémy Maucherat <remm@apache.org
> <ma...@apache.org>> wrote:
>
>
> Does the connector need to know about the actual implementations?
>
>
> Ideally no, but it removes the reflection you say is bad for Graal.
>
>
> Correct. Turns out that the connectors use setProperty/getProperty via
> reflection (IntrospectionUtils.setProperty/getProperty), so changing
> only this constructor would achieve a mini step.
> Before we commit any changes, I'd like to evaluate the scope of
> reflection we're dealing with.
>
> Then I can come back. I'll close the PR for now, as it only touches the
> surface.
>
> sound fair?
Sounds reasonable. I'm happy for any obvious clean-up to stay though.
On a similar note, the ProtocolHandler calls setProperty() on the
Endpoint which then also uses reflection.
I think I have a way around this but it is not great for maintenance.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: API Change - Connector.java Constructor
Posted by Filip Hanik <fh...@pivotal.io>.
On Tue, Apr 7, 2020 at 9:35 AM Rémy Maucherat <re...@apache.org> wrote:
>
>> Does the connector need to know about the actual implementations?
>>
>
> Ideally no, but it removes the reflection you say is bad for Graal.
>
Correct. Turns out that the connectors use setProperty/getProperty via
reflection (IntrospectionUtils.setProperty/getProperty), so changing only
this constructor would achieve a mini step.
Before we commit any changes, I'd like to evaluate the scope of reflection
we're dealing with.
Then I can come back. I'll close the PR for now, as it only touches the
surface.
sound fair?
Filip
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Apr 7, 2020 at 5:22 PM Filip Hanik <fh...@pivotal.io> wrote:
> On Tue, Apr 7, 2020 at 8:15 AM Rémy Maucherat <re...@apache.org> wrote:
>
>> On Tue, Apr 7, 2020 at 10:16 AM Mark Thomas <ma...@apache.org> wrote:
>>
>>> > Noted, I think a compromise may be in order. Where we simply add a
>>> > constructor that avoids the Class.forName
>>> > and that allows the developer to explicitly invoke a constructor that
>>> > avoids it.
>>>
>>> My thinking was more along the following lines...
>>>
>>> Rewrite the Connector (and possibly some/all other components) so that
>>> if a known class name was specified, rather than using the specified
>>> name directly via reflection we could do something like:
>>>
>>> if ("org.apache.coyote.http11.Http11NioProtocol".equals(className)) {
>>> protocol = new Http11NioProtocol();
>>> } else if ("org...
>>> ...
>>> } else {
>>> // OK. Unrecognised class name. Use reflection
>>> ...
>>> }
>>>
>>> Would that style of approach help at all?
>>>
>>
>> Proposed patch:
>> https://pastebin.com/ydCYnBD9
>>
>
> That patch works, I would include the check for the full classname too
>
> Was: if ("HTTP/1.1".equals(protocol) || protocol == null)
> Could be: if (protocol == null || "HTTP/1.1".equals(protocol) ||
> "HTTP/1.1".equals(Http11NioProtocol.class.getName))
>
> However, what still is awkward is that Connector.java deals with the
> interface ProtocolHandler.
> The loading of both the Http11NioConnector and the Ajp13NioConnector seems
> to defy the purpose of having that interface being present
>
> Is: protected final ProtocolHandler protocolHandler;
>
> Does the connector need to know about the actual implementations?
>
Ideally no, but it removes the reflection you say is bad for Graal.
Rémy
>
> Filip
>
>
>
>
>>
>> Rémy
>>
>>
>>>
>>> Mark
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>>
Re: API Change - Connector.java Constructor
Posted by Filip Hanik <fh...@pivotal.io>.
On Tue, Apr 7, 2020 at 8:15 AM Rémy Maucherat <re...@apache.org> wrote:
> On Tue, Apr 7, 2020 at 10:16 AM Mark Thomas <ma...@apache.org> wrote:
>
>> > Noted, I think a compromise may be in order. Where we simply add a
>> > constructor that avoids the Class.forName
>> > and that allows the developer to explicitly invoke a constructor that
>> > avoids it.
>>
>> My thinking was more along the following lines...
>>
>> Rewrite the Connector (and possibly some/all other components) so that
>> if a known class name was specified, rather than using the specified
>> name directly via reflection we could do something like:
>>
>> if ("org.apache.coyote.http11.Http11NioProtocol".equals(className)) {
>> protocol = new Http11NioProtocol();
>> } else if ("org...
>> ...
>> } else {
>> // OK. Unrecognised class name. Use reflection
>> ...
>> }
>>
>> Would that style of approach help at all?
>>
>
> Proposed patch:
> https://pastebin.com/ydCYnBD9
>
That patch works, I would include the check for the full classname too
Was: if ("HTTP/1.1".equals(protocol) || protocol == null)
Could be: if (protocol == null || "HTTP/1.1".equals(protocol) ||
"HTTP/1.1".equals(Http11NioProtocol.class.getName))
However, what still is awkward is that Connector.java deals with the
interface ProtocolHandler.
The loading of both the Http11NioConnector and the Ajp13NioConnector seems
to defy the purpose of having that interface being present
Is: protected final ProtocolHandler protocolHandler;
Does the connector need to know about the actual implementations?
Filip
>
> Rémy
>
>
>>
>> Mark
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
Re: API Change - Connector.java Constructor
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Apr 7, 2020 at 10:16 AM Mark Thomas <ma...@apache.org> wrote:
> > Noted, I think a compromise may be in order. Where we simply add a
> > constructor that avoids the Class.forName
> > and that allows the developer to explicitly invoke a constructor that
> > avoids it.
>
> My thinking was more along the following lines...
>
> Rewrite the Connector (and possibly some/all other components) so that
> if a known class name was specified, rather than using the specified
> name directly via reflection we could do something like:
>
> if ("org.apache.coyote.http11.Http11NioProtocol".equals(className)) {
> protocol = new Http11NioProtocol();
> } else if ("org...
> ...
> } else {
> // OK. Unrecognised class name. Use reflection
> ...
> }
>
> Would that style of approach help at all?
>
Proposed patch:
https://pastebin.com/ydCYnBD9
Rémy
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: API Change - Connector.java Constructor
Posted by Mark Thomas <ma...@apache.org>.
On 06/04/2020 20:50, Filip Hanik wrote:
> On Mon, Apr 6, 2020 at 11:20 AM Mark Thomas <markt@apache.org
> <ma...@apache.org>> wrote:
>
> On 06/04/2020 17:56, Filip Hanik wrote:
> > Team,
> >
> > As I'm slowly transitioning between projects, Apache Tomcat has once
> > again showed up in my workspace. I'm currently working on
> improving the
> > embedded experience for native images.
> >
> > I have a pull request,[1] <https://github.com/apache/tomcat/pull/267>,
> > that I'd like to open up a discussion about
> >
> > Goal: Able to start embedded Apache Tomcat without using reflection.
>
> Do you have a test case you are working to? It would be helpful to see
> exactly where the problems are in terms of what is considered reflection
> and what isn't.
>
>
> Yes, I'm working on a sample that should demonstrate it.
Excellent.
> On that point, can we tell the tooling that reflection isn't used even
> if the automated analysis can't tell that? I'm thinking of something
> along the lines of ensuring that most execution paths - including
> embedded - avoid reflection. We should, hopefully, be able to document
> the scenarios where reflection is triggered so users know to avoid them.
>
> > This PR: Changes the constructor for Connector to not use reflection.
> > Instead the calling components will instantiate the ProtocolHandler
> >
> > Besides the constructor change, functionality should remain backwards
> > compatible.
>
> I am very uncomfortable changing the constructor for a fundamental
> component like Connector this far into a stable release - hence wanting
> to explore options.
>
>
> Noted, I think a compromise may be in order. Where we simply add a
> constructor that avoids the Class.forName
> and that allows the developer to explicitly invoke a constructor that
> avoids it.
My thinking was more along the following lines...
Rewrite the Connector (and possibly some/all other components) so that
if a known class name was specified, rather than using the specified
name directly via reflection we could do something like:
if ("org.apache.coyote.http11.Http11NioProtocol".equals(className)) {
protocol = new Http11NioProtocol();
} else if ("org...
...
} else {
// OK. Unrecognised class name. Use reflection
...
}
Would that style of approach help at all?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: API Change - Connector.java Constructor
Posted by Filip Hanik <fh...@pivotal.io>.
On Mon, Apr 6, 2020 at 11:20 AM Mark Thomas <ma...@apache.org> wrote:
> On 06/04/2020 17:56, Filip Hanik wrote:
> > Team,
> >
> > As I'm slowly transitioning between projects, Apache Tomcat has once
> > again showed up in my workspace. I'm currently working on improving the
> > embedded experience for native images.
> >
> > I have a pull request,[1] <https://github.com/apache/tomcat/pull/267>,
> > that I'd like to open up a discussion about
> >
> > Goal: Able to start embedded Apache Tomcat without using reflection.
>
> Do you have a test case you are working to? It would be helpful to see
> exactly where the problems are in terms of what is considered reflection
> and what isn't.
>
Yes, I'm working on a sample that should demonstrate it.
> On that point, can we tell the tooling that reflection isn't used even
> if the automated analysis can't tell that? I'm thinking of something
> along the lines of ensuring that most execution paths - including
> embedded - avoid reflection. We should, hopefully, be able to document
> the scenarios where reflection is triggered so users know to avoid them.
>
> > This PR: Changes the constructor for Connector to not use reflection.
> > Instead the calling components will instantiate the ProtocolHandler
> >
> > Besides the constructor change, functionality should remain backwards
> > compatible.
>
> I am very uncomfortable changing the constructor for a fundamental
> component like Connector this far into a stable release - hence wanting
> to explore options.
>
Noted, I think a compromise may be in order. Where we simply add a
constructor that avoids the Class.forName
and that allows the developer to explicitly invoke a constructor that
avoids it.
Filip
Re: API Change - Connector.java Constructor
Posted by Mark Thomas <ma...@apache.org>.
On 06/04/2020 17:56, Filip Hanik wrote:
> Team,
>
> As I'm slowly transitioning between projects, Apache Tomcat has once
> again showed up in my workspace. I'm currently working on improving the
> embedded experience for native images.
>
> I have a pull request,[1] <https://github.com/apache/tomcat/pull/267>,
> that I'd like to open up a discussion about
>
> Goal: Able to start embedded Apache Tomcat without using reflection.
Do you have a test case you are working to? It would be helpful to see
exactly where the problems are in terms of what is considered reflection
and what isn't.
On that point, can we tell the tooling that reflection isn't used even
if the automated analysis can't tell that? I'm thinking of something
along the lines of ensuring that most execution paths - including
embedded - avoid reflection. We should, hopefully, be able to document
the scenarios where reflection is triggered so users know to avoid them.
> This PR: Changes the constructor for Connector to not use reflection.
> Instead the calling components will instantiate the ProtocolHandler
>
> Besides the constructor change, functionality should remain backwards
> compatible.
I am very uncomfortable changing the constructor for a fundamental
component like Connector this far into a stable release - hence wanting
to explore options.
Mark
> Please provide feedback, such as
> 1. Are we open to making API changes like this?
> 2. Does the PR introduce any problems?
>
> Thank you
> Filip
>
> [1] https://github.com/apache/tomcat/pull/267
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org