Skip to content
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

fix: don't bind Jetty if server.port is -1 #6141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Dogacel
Copy link
Contributor

@Dogacel Dogacel commented Mar 6, 2025

Motivation:

Make behavior in Tomcat and Jetty consistent when server.port is set to -1.

Modifications:

  • Configure JettyServlet to remove the binding to a random port if server.port is -1 and an armeria connector is added.

Result:

// Jetty servlet. See: https://github.com/line/armeria/issues/5039
Arrays.stream(jettyServer.getConnectors())
.filter(connector -> connector instanceof ServerConnector)
.filter(connector -> ((ServerConnector) connector).getPort() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) I was wondering what would happen if server.port: 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I don't think we can differentiate. Do you think we should open a separate issue to upstream?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a cleaner idea.
As a workaround, I'd like to add an option to close and remove ServerConnector in JettyService. It does not automatically work, though.
For example:

class JettyService {
    JettyService(..., boolean disableJettyServer) { ... }

    void start() throws Exception {
        if (disableJettyServer) {
            ...
            serverConnector.stop();
        }
    }
}

@Bean
public JettyService jettyService(ServletWebServerApplicationContext applicationContext,
                                 @Value("${server.port}") Integer port) throws Exception {
    final JettyWebServer jettyWebServer = jettyServer(applicationContext);
    boolean disableJettyServer == port != null && port == -1;
    return JettyService.of(jettyWebServer.getServer(), null, disableJettyServer);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand how we handle the -1 case for tomcat,

https://github.com/spring-projects/spring-boot/blob/3e4a9f5204ee2fe989583b17b77d8dab8558282a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactory.java#L326-L327

It seems like default behavior for both Jetty and Tomcat is the same, round it up to 0 and assign a random port.

Besides customizing the JettyService, what options do we have? Initial recommendation is to

We should override the getWebServer and fix it for consistent behavior with the Spring Tomcat integration.

However it would require us to copy-paste bunch of stuff, I am not sure if this is a pattern Armeria is willing to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it will take some time until the Spring folks figure out what to do on the upstream. Shall we proceed to introducing the workaround suggested by @ikhoon? That way, we could avoid unnecessary behavior changes when Spring is not involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it has been a busy couple weeks, travel, moving etc.

I can try to implement it 👍

@ikhoon ikhoon added the defect label Mar 6, 2025
@ikhoon ikhoon added this to the 1.33.0 milestone Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spring integration using Jetty opens a port event when server.port: -1
3 participants