Bug 60297

Summary: Tomcat.setConnector not using supplied connector
Product: Tomcat 9 Reporter: Axel U <ulrich.axel>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: enhancement    
Priority: P2    
Version: 9.0.0.M11   
Target Milestone: -----   
Hardware: All   
OS: All   
Attachments: Tomcat get/setConnector changes

Description Axel U 2016-10-22 02:47:59 UTC
Working on an embedded tomcat implementation, the following feedback:

org.apache.catalina.startup.Tomcat.setConnector(connector) 
In order to prevent the default connector to be created, one has to call setConnector(connector). 
Same applies if one wants to create his own ‘default’ connector, one has to call setConnector(connector) to prevent the default connector to be created so that later one can supply her own connector. 
But setting the connector with setConnector will not use the supplied connector as the default connector!  That is not intuitive.  
In fact, the connector that was set is never used in the code except being returned by the getConnector method.
Suggestion: setConnector should actually add the connector supplied, so something like this
        if (this.connector != null) {
            exit; // setting it again has no effect
        }
        getServer();
        service.addConnector( connector );
        this.connector = connector;
        return;
This will still introduce the issue of methods having to be called in a certain order.
Currently I have an implementation similar to the following to configure a custom nio2 and nio2 ssl connector.  
I should be able to call setConnector(connector) for at least one of those, but that is currently not possible.
Instead one has to call setConnector(connector) once to prevent the default connector to be created and then add the desired connectors.

        Tomcat tomcat = new Tomcat();
        …
        // Connector defaultConnector = getAprConnector();
        // Connector defaultConnector = getNioConnector()
        Connector defaultConnector = getNio2Connector();        
        tomcat.setConnector(defaultConnector); // will result in no connector
        …
        Service service = tomcat.getService();
        service.addConnector(defaultConnector); // adding ‘default’ connector
        // service.addConnector(getAprSslConnector());
        // service.addConnectory(getNioSslConnector());
        service.addConnector(getNio2SslConnector());


Additionally:
Even with the APR library available, the default connector is created as NIO and not as APR.
The reason is that AprLifecycleListener.useAprConnector is initialized to false, and never set to true.
So the comment in Tomcat class getConnector method 
        // This creates an APR HTTP connector if AprLifecycleListener has been
        // configured (created) and Tomcat Native library is available.
is misleading, unless when adding the AprLifecycleListener, one also sets useAprConnector to true
so something like this:
        StandardServer server = (StandardServer) tomcat.getServer();
        AprLifecycleListener listener = new AprLifecycleListener();
        listener.setUseAprConnector(true); // setting this will default it to the APR connector, otherwise NIO connector will be chosen as default
        server.addLifecycleListener(listener);
Not sure of a good solution, but the comment is misleading by itself and just adding the AprLifeCycleListener w/o knowing the internals has not the result one would expect, as it will use the Nio connector by default instead of Apr even in the presence of the APR / native lib.
Comment 1 Remy Maucherat 2016-10-24 14:54:45 UTC
Created attachment 34401 [details]
Tomcat get/setConnector changes

Ok, maybe it's not very intuitive, and I don't see the point of having a connector field in the Tomcat class.

This patch makes using setConnector optional, while getConnector will use the first connector of the service. setConnector will also add the connector if it's not been added already. Does this cover what you are talking about ? [I got really confused in the middle of it ...]
Comment 2 Axel U 2016-10-25 00:54:38 UTC
The changes made will do the trick in regards to using the supplied connector.  
It leaves the second part of my comment open though.  So, is there any harm if the getServer() method always adds an APR LifescyleListener as follows:
AprLifecycleListener listener = new AprLifecycleListener();
listener.setUseAprConnector(true); 
server.addLifecycleListener(listener);
This would create the default connector to APR if available, which is in line with the standard default config.
Comment 3 Remy Maucherat 2016-10-25 08:42:44 UTC
That already works, except there's no AprLifecycleListener. It's the same as the regular configuration, which has it in server.xml. Remove it and there won't be an APR connector used automatically.
Comment 4 Axel U 2016-10-25 11:37:04 UTC
Ok.
Comment 5 Remy Maucherat 2016-10-25 11:56:55 UTC
The fix will be in 9.0.0.M12 and 8.5.7. Should it be ported to 8.0.x and 7.0.x ? I'm hesitating since it's a behavior change.
Comment 6 Axel U 2016-10-26 01:28:32 UTC
I am suggesting not porting it to 8.0.x and 7.0.x.