Skip to content

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

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.

Sign up for GitHub

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...pring-boot-jetty/src/main/java/example/springframework/boot/jetty/HelloConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
public class HelloConfiguration {

/**
* Returns a new {@link HealthChecker} that marks the server as unhealthy when Tomcat becomes unavailable.
* Returns a new {@link HealthChecker} that marks the server as unhealthy when Jetty becomes unavailable.
*/
@Bean
public HealthChecker jettyHealthChecker(ServletWebServerApplicationContext applicationContext) {
Expand Down
2 changes: 1 addition & 1 deletion examples/spring-boot-jetty/src/main/resources/config/application.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
spring.profiles.active: local
# Prevent the embedded Tomcat from opening a TCP/IP port.
# Prevent the embedded Jetty from opening a TCP/IP port.
server.port: -1
---
spring.config.activate.on-profile: local
Expand Down
2 changes: 1 addition & 1 deletion examples/spring-boot-jetty/src/test/resources/application-testbed.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Prevent the embedded Tomcat from opening a TCP/IP port.
# Prevent the embedded Jetty from opening a TCP/IP port.
server.port: -1
---
armeria:
Expand Down
19 changes: 10 additions & 9 deletions ...boot3-jetty12/src/main/java/com/linecorp/armeria/spring/jetty/SpringJettyApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,20 @@
@SpringBootApplication
public class SpringJettyApplication {

/**
* Bean to configure Armeria Jetty service.
*/
@Bean
public ArmeriaServerConfigurator armeriaTomcat(WebServerApplicationContext applicationContext) {
public JettyWebServer jettyWebServer(WebServerApplicationContext applicationContext) {
final WebServer webServer = applicationContext.getWebServer();
if (webServer instanceof JettyWebServer) {
final Server jettyServer = ((JettyWebServer) webServer).getServer();

return serverBuilder -> serverBuilder.service("prefix:/jetty/api/rest/v1",
JettyService.of(jettyServer));
return ((JettyWebServer) webServer);
}
return serverBuilder -> {};
throw new IllegalStateException("The web server is not Jetty: " + webServer);
}

@Bean
public ArmeriaServerConfigurator armeriaJetty(JettyWebServer jettyWebServer) {
final Server jettyServer = jettyWebServer.getServer();
return serverBuilder -> serverBuilder.service("prefix:/jetty/api/rest/v1",
JettyService.of(jettyServer));
}

@Configuration(proxyBeanMethods = false)
Expand Down
2 changes: 2 additions & 0 deletions it/spring/boot3-jetty12/src/main/resources/config/application.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
server.port: -1
---
armeria:
ports:
- port: 0
Expand Down
11 changes: 11 additions & 0 deletions ...jetty12/src/test/java/com/linecorp/armeria/spring/jetty/SpringJettyApplicationItTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
import org.springframework.boot.test.web.client.TestRestTemplate;
import org.springframework.boot.web.embedded.jetty.JettyWebServer;
import org.springframework.context.ApplicationContext;
import org.springframework.test.context.ActiveProfiles;

Expand All @@ -44,6 +45,8 @@ class SpringJettyApplicationItTest {
private TestRestTemplate restTemplate;
@Inject
private GreetingController greetingController;
@Inject
private JettyWebServer jettyWebServer;

@BeforeEach
public void init() throws Exception {
Expand All @@ -69,6 +72,14 @@ void greetingShouldReturnDefaultMessage() throws Exception {
.contains("Hello, World!");
}

@Test
void greetingDirectlyToJettyShouldReturnDefaultMessage() throws Exception {
assertThat(restTemplate.getForEntity("http://localhost:" + jettyWebServer.getPort() + "/greeting",
Void.class)
.getStatusCode().value()).isEqualByComparingTo(404);

}

@Test
void greetingShouldReturnUsersMessage() throws Exception {
assertThat(restTemplate.getForObject("http://localhost:" + httpPort +
Expand Down
1 change: 0 additions & 1 deletion it/spring/boot3-jetty12/src/test/resources/application-testbed.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# This currently doesn't work. See https://github.com/line/armeria/issues/5039
server.port: -1
---
armeria:
Expand Down
18 changes: 18 additions & 0 deletions jetty/jetty12/src/main/java/com/linecorp/armeria/server/jetty/JettyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.eclipse.jetty.server.HttpStream;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.internal.HttpConnection;
import org.eclipse.jetty.util.Callback;
import org.slf4j.Logger;
Expand Down Expand Up @@ -161,6 +162,23 @@ void start() throws Exception {
connector = new ArmeriaConnector(jettyServer, armeriaServer);
jettyServer.addConnector(connector);

// Jetty assigns a random port if the port is set to -1. In order to disable Jetty from binding to a
// random port, we remove the connector added. This is useful as Armeria redirects requests to the
// 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 👍

.forEach(connector -> {
jettyServer.removeConnector(connector);
try {
final int localPort = ((ServerConnector) connector).getLocalPort();
logger.info("Stopping unintended Jetty on port: {}", localPort);
connector.stop();
} catch (Exception ignored) {
// Ignore the exception as we are stopping the unintended Jetty.
}
});

if (!jettyServer.isRunning()) {
logger.info("Starting an embedded Jetty: {}", jettyServer);
jettyServer.start();
Expand Down
Loading