-
Notifications
You must be signed in to change notification settings - Fork 939
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
base: main
Are you sure you want to change the base?
Conversation
// 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) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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,
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
Motivation:
Make behavior in Tomcat and Jetty consistent when
server.port
is set to -1.Modifications:
JettyServlet
to remove the binding to a random port ifserver.port
is -1 and an armeria connector is added.Result:
server.port: -1
#5039.server.port
is assigned to-1
.