JettyWebServer breaks Jetty ConnectionLimit feature

[baranchikovaleks]

Hello everyone!

In Spring Boot version >2.5.0 JettyWebServer started caching and removing http connectors directly, not via adding lifecycle bean (I suspect this is when it happened - df5f591#diff-7f526bfbdf69fd8f17c40e6583deb2ca69c12528c7730a37c815ec7605eb5e98R122)

So when I configure JettyServerCustomizer to limit connections like this

    @Bean
    public JettyServerCustomizer serverConnectionLimit() {
        return server -> server.addBean(new ConnectionLimit(1, server));
    }

it just does not work, because all the connectors were removed from the server
https://github.com/eclipse/jetty.project/blob/jetty-9.4.46.v20220331/jetty-server/src/main/java/org/eclipse/jetty/server/ConnectionLimit.java#L141

As a workaround, I can use another constructor

    @Bean
    public JettyServerCustomizer connectorsConnectionLimit() {
        return server -> server.addBean(new ConnectionLimit(1, server.getConnectors()));
    }

which works just fine, but I don't think this is an obvious behavior.

I created a small project to reproduce
https://github.com/baranchikovaleks/connection-limit-test

It contains a single test which fails on Spring Boot >2.5.0 and works for older versions.

[wilkinsona]

Thanks for the sample and for pinpointing the cause of the change in behavior.

Unfortunately, for things to work as we want with Jetty 10, we have to remove the connectors before calling start() rather than via a bean that's called as part of start() processing. This is due to this change and the eager opening of connections. If we only remove the connectors via a bean, they are available to be opened early which breaks our port clash detection as, in a server with multiple connectors, it's then impossible to determine which connector failed to open.

It would be possible to use different logic in JettyWebServer for removing the connectors, depending on whether its Jetty 9 or Jetty 10 that's being used. However, others may be relying on the current behavior and, given that this works when you retrieve the connectors from the server and pass them into ConnectionLimit, I think we should leave things as they are to avoid the risk of regression in a maintenance release.