Skip to content

Commit dff85ce

Browse files
jknackjknack
committed
server.port is ignored in 3.8.0
- side effect of running multiple app instances - print a warning when called multiple times - fix #3653
1 parent 926e90c commit dff85ce

File tree

7 files changed

+203
-31
lines changed

7 files changed

+203
-31
lines changed

‎jooby/src/main/java/io/jooby/AvailableSettings.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,4 +67,10 @@ public interface AvailableSettings {
6767
* Paths.get(System.getProperty("user.dir"), "tmp")</code>
6868
*/
6969
String TMP_DIR = "application.tmpdir";
70+
71+
/**
72+
* Print a warning when {@link Server#setOptions(ServerOptions)} is called multiple times. Default
73+
* value is: <code>true</code> set to <code>false</code> to disable.
74+
*/
75+
String SERVER_OPTIONS_WARN = "server.options.warn";
7076
}

‎jooby/src/main/java/io/jooby/Jooby.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1315,6 +1315,18 @@ public static void runApp(
13151315
for (var factory : provider) {
13161316
var app = createApp(executionMode, factory);
13171317
app.server = targetServer;
1318+
/*
1319+
When running a single app instance, there is no issue with server options, when multiple
1320+
apps set options a warning will be printed
1321+
*/
1322+
var options = app.serverOptions;
1323+
if (options == null) {
1324+
options = ServerOptions.from(app.getConfig()).orElse(null);
1325+
}
1326+
if (options != null) {
1327+
options.setServer(server.getName());
1328+
server.setOptions(options);
1329+
}
13181330
apps.add(app);
13191331
}
13201332
targetServer.start(apps.toArray(new Jooby[0]));

‎jooby/src/main/java/io/jooby/Server.java‎

Lines changed: 56 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ abstract class Base implements Server {
9696

9797
private final AtomicBoolean stopping = new AtomicBoolean();
9898

99+
private boolean defaultOptions;
100+
99101
protected void fireStart(@NonNull List<Jooby> applications, @NonNull Executor defaultWorker) {
100102
for (Jooby app : applications) {
101103
app.setDefaultWorker(defaultWorker).start(this);
@@ -110,10 +112,8 @@ protected void fireReady(@NonNull List<Jooby> applications) {
110112

111113
protected void fireStop(@NonNull List<Jooby> applications) {
112114
if (stopping.compareAndSet(false, true)) {
113-
if (applications != null) {
114-
for (Jooby app : applications) {
115-
app.stop();
116-
}
115+
for (Jooby app : applications) {
116+
app.stop();
117117
}
118118
}
119119
}
@@ -123,10 +123,42 @@ protected void addShutdownHook() {
123123
Runtime.getRuntime().addShutdownHook(new Thread(MutedServer.mute(this)::stop));
124124
}
125125
}
126+
127+
private ServerOptions options;
128+
129+
@Override
130+
public final ServerOptions getOptions() {
131+
if (options == null) {
132+
options = defaultOptions();
133+
defaultOptions = true;
134+
}
135+
return options;
136+
}
137+
138+
@Override
139+
public Server setOptions(@NonNull ServerOptions options) {
140+
if (this.options != null && !defaultOptions) {
141+
var warn =
142+
Boolean.parseBoolean(System.getProperty(AvailableSettings.SERVER_OPTIONS_WARN, "true"));
143+
if (warn) {
144+
LoggerFactory.getLogger(getClass())
145+
.warn(
146+
"Server options must be called once. To turn off this warning set the: `{}`"
147+
+ " system property to `false`",
148+
AvailableSettings.SERVER_OPTIONS_WARN);
149+
}
150+
}
151+
this.options = options;
152+
this.defaultOptions = false;
153+
return this;
154+
}
155+
156+
protected abstract ServerOptions defaultOptions();
126157
}
127158

128159
/**
129-
* Set server options.
160+
* Set server options. This method should be called once, calling this method multiple times will
161+
* print a warning.
130162
*
131163
* @param options Server options.
132164
* @return This server.
@@ -156,8 +188,8 @@ protected void addShutdownHook() {
156188
@NonNull Server start(@NonNull Jooby... application);
157189

158190
/**
159-
* Utility method to turn off odd logger. This help to ensure same startup log lines across server
160-
* implementations.
191+
* Utility method to turn off odd logger. This helps to ensure same startup log lines across
192+
* server implementations.
161193
*
162194
* <p>These logs are silent at application startup time.
163195
*
@@ -234,6 +266,16 @@ static boolean isAddressInUse(@Nullable Throwable cause) {
234266
* @return A server.
235267
*/
236268
static Server loadServer() {
269+
return loadServer(null);
270+
}
271+
272+
/**
273+
* Load server from classpath using {@link ServiceLoader}.
274+
*
275+
* @param options Optional server options.
276+
* @return A server.
277+
*/
278+
static Server loadServer(@Nullable ServerOptions options) {
237279
List<Server> servers =
238280
stream(
239281
spliteratorUnknownSize(
@@ -244,13 +286,18 @@ static Server loadServer() {
244286
throw new StartupException("Server not found.");
245287
}
246288
if (servers.size() > 1) {
247-
List<String> names =
289+
var names =
248290
servers.stream()
249291
.map(it -> it.getClass().getSimpleName().toLowerCase())
250292
.collect(Collectors.toList());
251293
var log = LoggerFactory.getLogger(servers.get(0).getClass());
252294
log.warn("Multiple servers found {}. Using: {}", names, names.get(0));
253295
}
254-
return servers.get(0);
296+
var server = servers.get(0);
297+
if (options != null) {
298+
options.setServer(server.getName());
299+
server.setOptions(options);
300+
}
301+
return server;
255302
}
256303
}

‎jooby/src/test/java/io/jooby/Issue3653.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* Jooby https://jooby.io
3+
* Apache License Version 2.0 https://jooby.io/LICENSE.txt
4+
* Copyright 2014 Edgar Espina
5+
*/
6+
package io.jooby;
7+
8+
import static org.junit.jupiter.api.Assertions.assertEquals;
9+
10+
import org.jetbrains.annotations.NotNull;
11+
import org.junit.jupiter.api.Test;
12+
import org.mockito.Mockito;
13+
import org.slf4j.Logger;
14+
import org.slf4j.LoggerFactory;
15+
16+
public class Issue3653 {
17+
18+
private static final ServerOptions defaultOptions = new ServerOptions();
19+
20+
private static class TestServer extends Server.Base {
21+
22+
@Override
23+
protected ServerOptions defaultOptions() {
24+
return defaultOptions;
25+
}
26+
27+
@NotNull @Override
28+
public String getName() {
29+
return "Test";
30+
}
31+
32+
@NotNull @Override
33+
public Server start(@NotNull Jooby... application) {
34+
return this;
35+
}
36+
37+
@NotNull @Override
38+
public Server stop() {
39+
return this;
40+
}
41+
}
42+
43+
@Test
44+
public void shouldNotWarnWhenDefaultOptionsAreSet() {
45+
try (var factory = Mockito.mockStatic(LoggerFactory.class)) {
46+
var server = new TestServer();
47+
var mockLogger = Mockito.mock(Logger.class);
48+
factory.when(() -> LoggerFactory.getLogger(TestServer.class)).thenReturn(mockLogger);
49+
assertEquals(defaultOptions, server.getOptions());
50+
server.setOptions(new ServerOptions());
51+
Mockito.verify(mockLogger, Mockito.times(0)).warn(Mockito.isA(String.class));
52+
}
53+
}
54+
55+
@Test
56+
public void shouldNotWarnWhenOptionsAreSetForFirstTime() {
57+
try (var factory = Mockito.mockStatic(LoggerFactory.class)) {
58+
var server = new TestServer();
59+
var mockLogger = Mockito.mock(Logger.class);
60+
factory.when(() -> LoggerFactory.getLogger(TestServer.class)).thenReturn(mockLogger);
61+
server.setOptions(new ServerOptions());
62+
Mockito.verify(mockLogger, Mockito.times(0)).warn(Mockito.isA(String.class));
63+
}
64+
}
65+
66+
@Test
67+
public void shouldWarnWhenOptionsAreSetMultipleTimes() {
68+
try (var factory = Mockito.mockStatic(LoggerFactory.class)) {
69+
var server = new TestServer();
70+
var mockLogger = Mockito.mock(Logger.class);
71+
factory.when(() -> LoggerFactory.getLogger(TestServer.class)).thenReturn(mockLogger);
72+
// first OK
73+
server.setOptions(new ServerOptions());
74+
Mockito.verify(mockLogger, Mockito.times(0)).warn(Mockito.isA(String.class));
75+
// Second warn
76+
server.setOptions(new ServerOptions());
77+
Mockito.verify(mockLogger, Mockito.times(1))
78+
.warn(
79+
"Server options must be called once. To turn off this warning set the: `{}` system"
80+
+ " property to `false`",
81+
AvailableSettings.SERVER_OPTIONS_WARN);
82+
}
83+
}
84+
85+
@Test
86+
public void shouldNotWarnWhenOptionsAreSetMultipleTimesWhenOptionIsOff() {
87+
try (var factory = Mockito.mockStatic(LoggerFactory.class)) {
88+
System.setProperty(AvailableSettings.SERVER_OPTIONS_WARN, "false");
89+
var server = new TestServer();
90+
var mockLogger = Mockito.mock(Logger.class);
91+
factory.when(() -> LoggerFactory.getLogger(TestServer.class)).thenReturn(mockLogger);
92+
// first OK
93+
server.setOptions(new ServerOptions());
94+
Mockito.verify(mockLogger, Mockito.times(0)).warn(Mockito.isA(String.class));
95+
// Second OK
96+
server.setOptions(new ServerOptions());
97+
Mockito.verify(mockLogger, Mockito.times(0)).warn(Mockito.isA(String.class));
98+
System.getProperties().remove(AvailableSettings.SERVER_OPTIONS_WARN);
99+
}
100+
}
101+
}

‎modules/jooby-jetty/src/main/java/io/jooby/jetty/JettyServer.java‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ public class JettyServer extends io.jooby.Server.Base {
5252

5353
private List<Jooby> applications;
5454

55-
private ServerOptions options = new ServerOptions().setServer("jetty").setWorkerThreads(THREADS);
5655
private Consumer<HttpConfiguration> httpConfigurer;
5756

5857
// TODO: integrate buffer factory with Jetty.
@@ -66,13 +65,13 @@ public JettyServer() {}
6665

6766
@NonNull @Override
6867
public JettyServer setOptions(@NonNull ServerOptions options) {
69-
this.options = options.setWorkerThreads(options.getWorkerThreads(THREADS));
68+
super.setOptions(options.setWorkerThreads(options.getWorkerThreads(THREADS)));
7069
return this;
7170
}
7271

73-
@NonNull @Override
74-
public ServerOptions getOptions() {
75-
return options;
72+
@Override
73+
protected ServerOptions defaultOptions() {
74+
return new ServerOptions().setServer("jetty").setWorkerThreads(THREADS);
7675
}
7776

7877
@NonNull @Override
@@ -93,6 +92,8 @@ public JettyServer configure(Consumer<HttpConfiguration> configurer) {
9392

9493
@NonNull @Override
9594
public io.jooby.Server start(@NonNull Jooby... application) {
95+
// force options to be non-null
96+
var options = getOptions();
9697
try {
9798
this.applications = List.of(application);
9899
/* Set max request size attribute: */
@@ -212,7 +213,7 @@ public io.jooby.Server start(@NonNull Jooby... application) {
212213
}
213214

214215
/* ********************************* Handler *************************************/
215-
var handlerList = createHandler(applications);
216+
var handlerList = createHandler(options, applications);
216217
Handler handler =
217218
handlerList.size() == 1 ? handlerList.get(0).getValue() : new PrefixHandler(handlerList);
218219
context.setHandler(handler);
@@ -257,7 +258,8 @@ public io.jooby.Server start(@NonNull Jooby... application) {
257258
return this;
258259
}
259260

260-
private List<Map.Entry<String, Handler>> createHandler(List<Jooby> applications) {
261+
private List<Map.Entry<String, Handler>> createHandler(
262+
ServerOptions options, List<Jooby> applications) {
261263
return applications.stream()
262264
.map(
263265
application -> {

‎modules/jooby-netty/src/main/java/io/jooby/netty/NettyServer.java‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,6 @@ public class NettyServer extends Server.Base {
5656
private NettyDataBufferFactory bufferFactory;
5757
private List<Jooby> applications;
5858

59-
private ServerOptions options = new ServerOptions().setServer("netty");
60-
6159
/**
6260
* Creates a server.
6361
*
@@ -93,13 +91,13 @@ public NettyServer() {}
9391

9492
@Override
9593
public @NonNull NettyServer setOptions(@NonNull ServerOptions options) {
96-
this.options = options;
94+
super.setOptions(options);
9795
return this;
9896
}
9997

100-
@NonNull @Override
101-
public ServerOptions getOptions() {
102-
return options;
98+
@Override
99+
protected ServerOptions defaultOptions() {
100+
return new ServerOptions().setServer("netty");
103101
}
104102

105103
@NonNull @Override
@@ -109,6 +107,8 @@ public String getName() {
109107

110108
@NonNull @Override
111109
public Server start(@NonNull Jooby... application) {
110+
// force options to be non-null
111+
var options = getOptions();
112112
try {
113113
this.applications = List.of(application);
114114
boolean single = applications.size() == 1;
@@ -142,7 +142,8 @@ public Server start(@NonNull Jooby... application) {
142142
var http2 = options.isHttp2() == Boolean.TRUE;
143143
/* Bootstrap: */
144144
if (!options.isHttpsOnly()) {
145-
var http = newBootstrap(allocator, transport, newPipeline(null, dateService, http2));
145+
var http =
146+
newBootstrap(allocator, transport, newPipeline(options, null, dateService, http2));
146147
http.bind(options.getHost(), options.getPort()).get();
147148
}
148149

@@ -154,7 +155,9 @@ public Server start(@NonNull Jooby... application) {
154155

155156
var clientAuth = sslOptions.getClientAuth();
156157
var sslContext = wrap(javaSslContext, toClientAuth(clientAuth), protocol, http2);
157-
var https = newBootstrap(allocator, transport, newPipeline(sslContext, dateService, http2));
158+
var https =
159+
newBootstrap(
160+
allocator, transport, newPipeline(options, sslContext, dateService, http2));
158161
https.bind(options.getHost(), options.getSecurePort()).get();
159162
} else if (options.isHttpsOnly()) {
160163
throw new IllegalArgumentException(
@@ -193,7 +196,7 @@ private ClientAuth toClientAuth(SslOptions.ClientAuth clientAuth) {
193196
}
194197

195198
private NettyPipeline newPipeline(
196-
SslContext sslContext, NettyDateService dateService, boolean http2) {
199+
ServerOptions options, SslContext sslContext, NettyDateService dateService, boolean http2) {
197200
var bufferSize = options.getBufferSize();
198201
var headersFactory = HeadersMultiMap.httpHeadersFactory();
199202
var decoderConfig =

0 commit comments

Comments
 (0)