Avoid duplicate URI construction in ReactorServerHttpRequest #30047
Conversation
Two URI's were being constructed. The first one was then converted to a string and then concatenated with the path and then back to a URI. This change only does a single phase of parsing.
spring-projects-issues
added
the
status: waiting-for-triage
rstoyanchev
added
the
in: web
There was a problem hiding this comment.
I don't mind eliminating one additional URI construction conceptually, but the performance improvement is negligible, and at the same time, as per my comments below, there is a level of validation lost, and some functionality is removed that should not be.
Thanks for the pull request, but overall I'm not inclined to accept the change.
| } | ||
| CharSequence header = request.requestHeaders().get(HttpHeaderNames.HOST); | ||
| if (header != null) { | ||
| return header; |
There was a problem hiding this comment.
The "Host" header can contain host and port, see for example https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host. And its parsing needs to take into account that the host maybe an IPv6 address. This is what the removed code [89-110] does, and that needs to remain.
| InetSocketAddress hostAddress = request.hostAddress(); | ||
| if (hostAddress != null) { | ||
| return new URI(scheme, null, hostAddress.getHostString(), hostAddress.getPort(), null, null, null); | ||
| return NetUtil.toSocketAddressString(hostAddress); |
There was a problem hiding this comment.
Before this change, the URI constructor that accepts individual URI components would validate the host and port. After this change, the value returned from resolveHost is not validated, the URI constructor with a String (called on line 79) doesn't either.
When I tested whether this change would help with spring-projects/spring-boot#34395 where X-Forwarded-HOST is "localhost:3000" and X-Forwarded-Port is "3000", I saw that resolveHost returns "localhost:3000:3000", and the resulting URI was "http://localhost:3000:3000/hello". When I test without this change, there is an exception from the URI constructor that was previously used here because "localhost:3000" is not a valid host.
|
ok i'll put the validations back, but i'm still going to try to do the URI parse once.
|
|
i'm going to close this until #30033 is resolved |
rstoyanchev
removed
the
status: waiting-for-triage
It appeared that two URI's were being constructed. The first one was then manually created then converted to a string and then concatenated with the path and then back to a URI. This change only does a single phase of parsing. Additionally, ipv6 InetSocketAddress instances will properly be converted to hostnames with the proper bracket format. I did the change in current and future reactor netty implementations.