Working on #36103 showed that there are some inconsistencies in how we log the Servlet WebServer's context path:
- Jetty logs
'/'by default - Tomcat logs
''by default - Undertow logs nothing by default
There are also some similar inconsistencies on the reactive side where there is no (configurable) context path:
- Jetty logs
'/' - Tomcat logs
'' - Undertow logs nothing
- Netty logs nothing
We should try to make things as consistent as possible, removing as many of the optional regex groups as possible from the tests added in #36103.
Comment From: wilkinsona
For Servlet WebServers I think we should always log something, even with the default empty context path. It would also be good to log '' or '/' consistently for this case.
For reactive WebServers I think it probably makes sense not to log anything at all as there isn't really a context path in this case. Technically, you could configure one for some of the servers, for example with a TomcatContextCustomizer when using Tomcat, but I'm not sure we should account for that in the log messages. I'd like to see what the rest of the team thinks about this though.
Comment From: philwebb
We're going with "Tomcat started on ports 8080 (http) with context path '/'" for servlets and "Tomcat started on ports 8080 (http)" for reactive.
Comment From: emileplas
@wilkinsona : if you wouldn't mind, I would try to have a go at this PR.
Comment From: vicaba
Hi @wilkinsona, @emileplas , I am trying to start contributing to spring boot. I thought this could be a great issue to start with as it seems easy. If @emileplas hasn't done it yet, I think I can start.
First time contributing to an open source project, so I may need a little help.
From #36103 , I have identified that the change is needed in the following files:
* spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyWebServer.java
* spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/netty/NettyWebServer.java
* spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatWebServer.java
* spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/UndertowWebServer.java
I have also identified the code that needs to be changed. At least the "end" code, I guess I will have to reverse engineer things to see where the issue comes form. However, I would like to know which branch I should be working on (base my PR branch on), because now I'm looking at the main branch and I'm not sure that this is the correct one.
Comment From: philwebb
Hi @vicaba, thanks for offering to pick this one up if @emileplas doesn't mind (let's give him a few days to respond).
I would like to know which branch I should be working on
We'd consider this one an enhancement so it only needs to be applied to the main branch.
Comment From: emileplas
@vicaba go ahead.
Comment From: vicaba
I have some news on this issue.
- I have found several smoke tests where I can try and visually check the changes. However, there is no smoke test for
netty. These are the smoke tests: spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-jetty/src/main/java/smoketest/jetty/SampleJettyApplication.java- There is no
spring-boot-smoke-test-netty? spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-tomcat/src/main/java/smoketest/tomcat/SampleTomcatApplication.java-
spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-undertow/src/main/java/smoketest/undertow/SampleUndertowApplication.java -
Regarding
jetty, I've used it as the model to follow. - I've not tackled
nettyyet, since there are no tests and maybe I need to create them, that's for you to decide (we may need another issue for that?). - I've already fixed the
Tomcatserver. - The
Undertowserver is more complicated. There is no trace of context path in theUndertowWebServerclass, and the context can't be retrieved on build phase by design, only provided via aHandler. As far as I understand, it can only be retrieved when handling requests. Also, the only trace of context path is inUndertowServletWebServer(and associated factory), where they do use theHandlerto provide the context path. So... providing the context path to theUndertowWebServerwould need modification of several classes related to the creation of theUndertowWebServer, which could imply breaking changes/me breaking something (Not sure there are proper tests for that). - Finally, should I create the PR from my branch?
Comment From: philwebb
Hi @vicaba, thanks for the update.
I've not tackled netty yet, since there are no tests and maybe I need to create them, that's for you to decide (we may need another issue for that?).
We can leave Netty for now. One of the team can always pick that up when we look at merging the PR.
Finally, should I create the PR from my branch?
Yes please. We probably won't get around to reviewing it until the new year as we've not yet branched for 3.3 development.
Comment From: wilkinsona
I've not tackled netty yet, since there are no tests
FWIW, we do have tests for Netty, including smoke tests. spring-boot-smoke-test-webflux is one.
Comment From: vicaba
Hi @philwebb, @wilkinsona, thank you for the info, I will take a look at the Netty situation this weekend and open the PR.
Comment From: wilkinsona
While we're doing this, we can also improve the logging of the protocol (http or https) across the four different web servers.