-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add Lazy cache for originalhost #1766
Conversation
@@ -510,12 +514,23 @@ protected String generateInfoForLogging() { | |||
@Override | |||
public String getOriginalHost() { | |||
try { | |||
return getOriginalHost(getHeaders(), getServerName()); | |||
if (!CACHE_ORIGINAL_HOST.get()) { | |||
return generateOriginalHost(getHeaders(), getServerName()); |
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'm not sure we need this FP to not-cache the original host; since it's intended for fetching the host on the inbound request, it shouldn't be mutated by any header processing, so keeping it always cached makes it simpler
} catch (URISyntaxException e) { | ||
throw new IllegalArgumentException(e); | ||
} | ||
} | ||
|
||
String generateOriginalHost(Headers headers, String serverName) throws URISyntaxException { |
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.
Think you can remove this method and just call getOriginalHost(headers, serverName)
from within the getOriginalHost()
method
@@ -510,7 +511,10 @@ protected String generateInfoForLogging() { | |||
@Override | |||
public String getOriginalHost() { | |||
try { | |||
return getOriginalHost(getHeaders(), getServerName()); | |||
if (originalHost == null) { | |||
originalHost = getOriginalHost(getHeaders(), getServerName()); |
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.
The compute intensive part here should be the parseHostHeader
logic.
Consider the option to just cache that part and not the X-Forwarded-Host
part.
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.
X-Forwarded-Host
part is called everytime before the parseHostHeader
and it loops on all existing headers to check for the x-forwarded-host
using headers.getFirst
before attempting to call the parseHostHeader
. Doesn't sound efficient. I am tempted to cache that as well.
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.
hmm, it does iterate through the list, ugh.
* Lazy cache originalhost * Update toString function * Update unittests --------- Co-authored-by: $(git --no-pager log --format=format:'%an' -n 1) <$(git --no-pager log --format=format:'%ae' -n 1)>
Add Lazy cache for originalhost when parsing headers