WebFlux app can't start if management.server.port is customized #2980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2980 +/- ##
============================================
+ Coverage 73.22% 73.23% +0.01%
- Complexity 12280 12292 +12
============================================
Files 1060 1060
Lines 47681 47695 +14
Branches 6021 6027 +6
============================================
+ Hits 34914 34930 +16
+ Misses 9715 9714 -1
+ Partials 3052 3051 -1 Continue to review full report at Codecov.
|
minwoox
reviewed
Aug 10, 2020
|
|
||
| private boolean isManagementPortEqualsToServerPort() { | ||
| final Integer managementPort = environment.getProperty("management.server.port", Integer.class); | ||
| if (managementPort != null && managementPort < 0) { |
There was a problem hiding this comment.
How about calling ensureValidPort(managementPort) so that an exception is raised if it's under 0?
There was a problem hiding this comment.
Also, how about returning early if managementPort is null?
| @@ -131,6 +140,11 @@ public ArmeriaReactiveWebServerFactory(ConfigurableListableBeanFactory beanFacto | |||
|
|
|||
| @Override | |||
| public WebServer getWebServer(HttpHandler httpHandler) { | |||
| final int port = ensureValidPort(getPort()); | |||
| if (armeriaWebServer != null && needsToReuseWebServer(port)) { | |||
There was a problem hiding this comment.
If armeriaWebServer is not null and the port is different, it seems like the previously configured settings to armeriaWebServer are gone. Isn't it right?
There was a problem hiding this comment.
The previously configured settings remains.
I'll add some tests to check this.
|
updated. |
minwoox
approved these changes
Aug 11, 2020
| final ArmeriaWebServer armeriaWebServer = new ArmeriaWebServer(server, protocol, address, port, | ||
| beanFactory); | ||
| if (!isManagementPortEqualsToServerPort()) { | ||
| // Since this method will be called twice, need to reuse ArmeriaWebServer |
There was a problem hiding this comment.
How about leaving a comment?
The management port is set to the Server in ArmeriaSpringActuatorAutoConfiguration.
|
Updated the comment. |
trustin
approved these changes
Aug 12, 2020
ikhoon
reviewed
Aug 12, 2020
| "65536", | ||
| }) | ||
| void isManagementPortEqualsToServerPortThrows(String managementPort) { | ||
| assertThrows(IllegalArgumentException.class, () -> { |
There was a problem hiding this comment.
Could use assertThatThrownBy(...)
| .path(0) | ||
| .textValue()) | ||
| .isEqualTo("/hello/foo"); | ||
| } catch (JsonProcessingException e) { |
|
@matsumana Let me merge once @ikhoon's comments are addressed. |
|
updated! |
|
Thanks a lot, @matsumana |
|
Thank you guys! |
matsumana
deleted the
fix-management-port-issue-for-boot2-webflux-autoconfigure
branch
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
…#2980) If users customize WebFlux app's `management.server.port` setting as follows, the app can't start. ```yaml # e.g. management: server: port: 18080 ``` Because if `management.server.port` is not equal to `server.port`, [DifferentManagementContextConfiguration](https://github.com/spring-projects/spring-boot/blob/v2.3.2.RELEASE/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/web/server/ManagementContextAutoConfiguration.java#L115-L116) will be enabled then `ArmeriaReactiveWebServerFactory#getWebServer(HttpHandler)` will be called twice. The process flow is the following. 1st call of `ArmeriaReactiveWebServerFactory#getWebServer(HttpHandler)`: - https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-context/src/main/java/org/springframework/context/support/AbstractApplicationContext.java#L554 - https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-context/src/main/java/org/springframework/context/support/AbstractApplicationContext.java#L895 - https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-context/src/main/java/org/springframework/context/support/DefaultLifecycleProcessor.java#L122 - https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-context/src/main/java/org/springframework/context/support/DefaultLifecycleProcessor.java#L158 - https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-context/src/main/java/org/springframework/context/support/DefaultLifecycleProcessor.java#L360 - https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-context/src/main/java/org/springframework/context/support/DefaultLifecycleProcessor.java#L182 - https://github.com/spring-projects/spring-boot/blob/v2.3.2.RELEASE/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/WebServerStartStopLifecycle.java#L40 - https://github.com/spring-projects/spring-boot/blob/v2.3.2.RELEASE/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/WebServerManager.java#L54 - https://github.com/line/armeria/blob/armeria-0.99.9/spring/boot2-webflux-autoconfigure/src/main/java/com/linecorp/armeria/spring/web/ArmeriaWebServer.java#L82 Then, 2nd time ApplicationContext refreshing will be started by `DifferentManagementContextConfiguration#onApplicationEvent(WebServerInitializedEvent)`. - https://github.com/spring-projects/spring-boot/blob/v2.3.2.RELEASE/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/WebServerManager.java#L55-L56 - https://github.com/spring-projects/spring-boot/blob/v2.3.2.RELEASE/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/web/server/ManagementContextAutoConfiguration.java#L131-L142 I investigated whether I can stop ApplicationContext refreshing by `DifferentManagementContextConfiguration#onApplicationEvent(WebServerInitializedEvent)`. But I couldn't find a way to customize. So I added ArmeriaWebServer field into ArmeriaReactiveWebServerFactory to cache it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Hi.
If users customize WebFlux app's
management.server.portsetting as follows, the app can't start.Because if
management.server.portis not equals toserver.port, DifferentManagementContextConfiguration will be enabled thenArmeriaReactiveWebServerFactory#getWebServer(HttpHandler)will be called twice.The process flow is the following.
1st call of
ArmeriaReactiveWebServerFactory#getWebServer(HttpHandler):Then, 2nd time ApplicationContext refreshing will be started by
DifferentManagementContextConfiguration#onApplicationEvent(WebServerInitializedEvent).I investigated whether I can stop ApplicationContext refreshing by
DifferentManagementContextConfiguration#onApplicationEvent(WebServerInitializedEvent).But I couldn't find a way to customize.
So I added ArmeriaWebServer field into ArmeriaReactiveWebServerFactory to cache it.