Reuse VirtualHostBuilder on the same hostnamePattern #5418

yejinio · 2024-01-29T05:26:18Z

Motivation:

I registered each virtual host with the same hostname, the registered routes cannot be not found. Because VirtualHostBuilder creates new builder even if hostname is the same. However, it does reuse them on the same port.
So, I think it's better to reuse them on the same hostname.

Modifications:

ServerBuilder.virtualHost(hostnamePattern) and ServerBuilder.virtualHost(defaultHostname, hostnamePattern) checks whether there is builders with the same hostname. If there is, returns this builder.

Result:

Reuse VirtualHostBuilder on the same hostname.
Even if you register each one on a virtual host with the same hostname, they will be merged.

CLAassistant · 2024-01-29T05:26:23Z

All committers have signed the CLA.

github-actions · 2024-01-30T07:04:09Z

🔍 Build Scan® (commit: `33630fa`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/ipk5uvwhx7mqg
build-self-hosted-unsafe-jdk-8	❌ (failure)	https://ge.armeria.dev/s/hitgjcglcsyee
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/yhbdtw5o2clse
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/34cbrqddglqli
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/3jlcdhfdi25oi
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/6hrvtkpzq5g4c
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/xzeyacjvivcn2

ikhoon

Overall, it looks great.

ikhoon · 2024-02-07T06:19:00Z

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java

+        return normalizeDefaultHostname(defaultHostname).equals(this.defaultHostname);
+    }
+
+    boolean equalsHostnamePattern(String hostnamePattern) {


What do you think of parsing hostnamePattern before calling this method? Otherwise, the same hostnamePattern will be parsed as many virtual hosts.

Suggested change

boolean equalsHostnamePattern(String hostnamePattern) {

boolean equalsHostnamePattern(String validhostnamePattern, int port) {

Thank you for the advice. Before I modify it, I'd like to know your intend exactly.
If I modify as you suggest, hostnamePattern is parsed using HostAndPort by the part which calls this method and passed. (e.g. ServiceBuilder.virtualHost(String hostnamePattern)), is it correct?

@ikhoon I applied your comment as I understand. PTAL 🙏

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java

codecov · 2024-02-07T06:58:49Z

Codecov Report

Attention: Patch coverage is 80.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 74.09%. Comparing base (8678567) to head (283b7ec).
Report is 16 commits behind head on main.

Files	Patch %	Lines
...om/linecorp/armeria/server/VirtualHostBuilder.java	63.63%	1 Missing and 3 partials ⚠️
.../java/com/linecorp/armeria/server/VirtualHost.java	62.50%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5418      +/-   ##
============================================
+ Coverage     74.04%   74.09%   +0.04%     
- Complexity    20871    20986     +115     
============================================
  Files          1809     1819      +10     
  Lines         76857    77332     +475     
  Branches       9809     9880      +71     
============================================
+ Hits          56912    57300     +388     
- Misses        15304    15375      +71     
- Partials       4641     4657      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

minwoox

Hey~ thanks for the PR and good to see you here. 😆
Left some suggestions. PTAL.

minwoox · 2024-02-21T06:40:14Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+        final Optional<VirtualHostBuilder> vhost =
+                virtualHostBuilders.stream()
+                                   .filter(v -> !v.defaultVirtualHost() &&
+                                                v.equalsHostnamePattern(hostAndPort.getHost(),


The hostname is normalized thus this won't still find the builder if the name contains unicode.

armeria/core/src/main/java/com/linecorp/armeria/server/VirtualHost.java

Line 197 in 3adf832

hostnamePattern = IDN.toASCII(hostnamePattern, IDN.ALLOW_UNASSIGNED);

So how about validating and normalizing it before we find it?
Then we can just set the hostnamePattern and port directly to VirtualHostBuilder not to call HostAndPort.fromString() and validate it again.

Good idea. But, I don't fully understand your suggestion. Could you check I understand is correct?

Then we can just set the hostnamePattern and port directly to VirtualHostBuilder not to call HostAndPort.fromString() and validate it again.

validate and normalize in ServerBuilder.virtualHost(hostnamePattern)

if exists on same hostname, return it

if not exists, VirtualHostBuilder.hostnamePattern(hostnamePattern, port) calls

VirtualHostBuilder.hostnamePatter(hostnamePattern, port) sets directly hostnamePattern and port without validation and normalization

In 4, I'm worried about the hostnamePattern is able to be set without validation somewhere.

In 4, I'm worried about the hostnamePattern is able to be set without validation somewhere.

That's a good question. I believe that if we make the method package-private, we don't have to worry about it because a user can't just use the method to set the properties directly.
If you worry about it, we can also add the validation logic in the method but I don't believe we really need it. 😉

Oh, I didn't think about package-private! great job 👍 Thank you for the suggestion!

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

minwoox · 2024-02-24T13:05:43Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+        final int port = hostAndPort.getPortOrDefault(-1);
+        for (VirtualHostBuilder virtualHostBuilder : virtualHostBuilders) {
+            if (!virtualHostBuilder.defaultVirtualHost() &&
+                virtualHostBuilder.equalsDefaultHostname(defaultHostname) &&


What are your thoughts on utilizing hostnamePattern and port for the equality check and removing the defaultHostname check? Since hostnamePattern is solely used to identify the virtual host, I believe we can consider two virtual hosts as identical if their hostnamePattern matches.

armeria/core/src/main/java/com/linecorp/armeria/server/ServiceRouteUtil.java

Line 52 in 6e485eb

return DefaultRoutingContext.of(serverConfig.findVirtualHost(hostname, port),

armeria/core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java

Line 300 in 4121cf5

mappingBuilder.add(virtualHost.hostnamePattern(), virtualHost);

What do you think of raising an exception if a defaultHostname is registered for the same hostnamePattern before?
It may be controversial that defaultHostname is ignored silently.

That's a good suggestion. I think we can

set the defaultHostname if the previous virtualHostBuilder does not have the defaultHostname

raise an exception if the defaultHostname of the previous virtualHostBuilder not null and It's not the same with the parameter defaultHostname

A couple thoughts regarding the proposal:

Even before this PR, a defaultHostname could be ignored if there were multiple virtual hosts with the same hostnamePattern

The suggested behavior tries to guarantee uniqueness of (hostName, defaultHostname, port). However, the mutability of defaultHostname, hostnamePattern, port does not guarantee this.

Rather, I propose:

All methods that can mutate hostnamePattern are deprecated

VirtualHostBuilder#hostnamePattern(String) is deprecated

All methods that do not receive a single hostnamePattern or port are deprecated

ServerBuilder#virtualHost(String,String) is deprecated, since the additional defaultHostname makes it unclear which VirtualHostBuilder is returned.

ServerBuilder#withVirtualHost(Customizer) is deprecated. It is not compatible with the VirtualHostBuilder reuse functionality. If needed, we could add a ServerBuilder#withVirtualHost(String, Customizer), ServerBuilder#withVirtualHost(int,Customizer) instead.

This would make it clear that only hostnamePattern or port will be used for checking reusability of VirtualHostBuilder.

In terms of implementation plan, I propose we assume that the @Deprecated methods do not support VirtualHostBuilder reusability. We can simply remove the deprecated methods in the next major release which gives a straightforward upgrade plan as well.

Regarding this PR, I propose that this method is simply deprecated and we only support ServerBuilder#virtualHost(String) instead.

Thoughts @line/dx ?

That's a good idea. 👍 It's time to cleanup the API.

That sounds great to me.

If needed, we could add a ServerBuilder#withVirtualHost(String, Customizer)

It would be better than deprecation without a replacement. Some forks prefer explicit code blocks over the fluent style.

I'm sorry for late checking.

Regarding this PR, I propose that this method is simply deprecated and we only support ServerBuilder#virtualHost(String) instead.

I understand that I need to apply changes to ServerBuilder#virtualHost(String) in this PR. I'll remove the changes of ServerBuilder#virtualHost(String, String).

I'll remove the changes of ServerBuilder#virtualHost(String, String).

Thanks!

jrhee17

Thanks for the effort! Looks good overall 👍 👍 👍

jrhee17 · 2024-03-19T08:35:11Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+        final int port = hostAndPort.getPortOrDefault(-1);
+        for (VirtualHostBuilder virtualHostBuilder : virtualHostBuilders) {
+            if (!virtualHostBuilder.defaultVirtualHost() &&
+                virtualHostBuilder.equalsDefaultHostname(defaultHostname) &&


A couple thoughts regarding the proposal:

Even before this PR, a defaultHostname could be ignored if there were multiple virtual hosts with the same hostnamePattern

The suggested behavior tries to guarantee uniqueness of (hostName, defaultHostname, port). However, the mutability of defaultHostname, hostnamePattern, port does not guarantee this.

Rather, I propose:

All methods that can mutate hostnamePattern are deprecated

VirtualHostBuilder#hostnamePattern(String) is deprecated

All methods that do not receive a single hostnamePattern or port are deprecated

ServerBuilder#virtualHost(String,String) is deprecated, since the additional defaultHostname makes it unclear which VirtualHostBuilder is returned.

ServerBuilder#withVirtualHost(Customizer) is deprecated. It is not compatible with the VirtualHostBuilder reuse functionality. If needed, we could add a ServerBuilder#withVirtualHost(String, Customizer), ServerBuilder#withVirtualHost(int,Customizer) instead.

This would make it clear that only hostnamePattern or port will be used for checking reusability of VirtualHostBuilder.

In terms of implementation plan, I propose we assume that the @Deprecated methods do not support VirtualHostBuilder reusability. We can simply remove the deprecated methods in the next major release which gives a straightforward upgrade plan as well.

Regarding this PR, I propose that this method is simply deprecated and we only support ServerBuilder#virtualHost(String) instead.

Thoughts @line/dx ?

jrhee17 · 2024-03-28T02:39:52Z

gentle ping @yejinio 🙇

jrhee17 · 2024-04-02T07:15:07Z

I've added a small commit which

Deprecates ServerBuilder#virtualHost methods that doesn't support VirtualHostBuilder reuse
Adds a ServerBuilder#withVirtualHost which supports VirtualHostBuilder reuse
Deprecates VirtualHostBuilder#hostnamePattern which makes VirtualHostBuilder reuse ambiguous

Let me know if the changes don't make sense 🙇
0d0edef

jrhee17

Changes look good to me 👍 👍 👍

ikhoon

@yejinio Thanks for your contribution! 🚀🚀

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

minwoox

@yejinio Thank you very much for all your hard work! 🙇

yejinio requested review from ikhoon, jrhee17, minwoox and trustin as code owners January 29, 2024 05:26

yejinio force-pushed the reuse-vhost-with-hostname branch from aab4ec5 to 906334e Compare January 29, 2024 05:29

ikhoon added the defect label Jan 29, 2024

ikhoon added this to the 1.28.0 milestone Jan 29, 2024

yejinio force-pushed the reuse-vhost-with-hostname branch from 906334e to 90a50c6 Compare January 30, 2024 08:39

ikhoon reviewed Feb 7, 2024

View reviewed changes

yejinio requested a review from ikhoon February 13, 2024 14:10

yejinio force-pushed the reuse-vhost-with-hostname branch from fc34ae2 to 7e57f3d Compare February 15, 2024 09:21

minwoox reviewed Feb 21, 2024

View reviewed changes

minwoox reviewed Feb 24, 2024

View reviewed changes

jrhee17 reviewed Mar 19, 2024

View reviewed changes

yejinio added 6 commits March 29, 2024 11:35

Reuse VirtualHostBuilder on the same hostnamePattern

634b3d3

Check port is the same if builder has port

c8fb83e

Parse hostnamePattern before checking equals

426f113

Make simplify using for-loop

8118baf

Validate and normalize before finding on same hostname

2203b0c

Remove defaultHostname changes

224907a

yejinio force-pushed the reuse-vhost-with-hostname branch from 1f4ca74 to 224907a Compare March 29, 2024 02:49

Add unittest for checking defaultVirtualHost

0b51cb7

yejinio requested review from minwoox and jrhee17 March 29, 2024 03:05

deprecate methods

0d0edef

jrhee17 approved these changes Apr 2, 2024

View reviewed changes

ikhoon approved these changes Apr 4, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java Show resolved Hide resolved

ikhoon and others added 2 commits April 4, 2024 16:13

Update core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

c517ab1

Deprecate withVirtualHost

283b7ec

minwoox approved these changes Apr 4, 2024

View reviewed changes

Merge branch 'main' into reuse-vhost-with-hostname

33630fa

jrhee17 merged commit dad02d6 into line:main Apr 4, 2024
6 of 7 checks passed

jrhee17 added the deprecation label Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse VirtualHostBuilder on the same hostnamePattern #5418

Reuse VirtualHostBuilder on the same hostnamePattern #5418

yejinio commented Jan 29, 2024

CLAassistant commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 30, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Feb 7, 2024

yejinio Feb 8, 2024

yejinio Feb 15, 2024

codecov bot commented Feb 7, 2024 •

edited

Loading

minwoox left a comment

minwoox Feb 21, 2024

yejinio Feb 23, 2024 •

edited

Loading

minwoox Feb 23, 2024 •

edited

Loading

yejinio Feb 23, 2024

minwoox Feb 24, 2024

ikhoon Mar 18, 2024

minwoox Mar 18, 2024

jrhee17 Mar 19, 2024

minwoox Mar 20, 2024

ikhoon Mar 20, 2024

yejinio Mar 28, 2024 •

edited

Loading

jrhee17 Mar 29, 2024

jrhee17 left a comment

jrhee17 Mar 19, 2024

jrhee17 commented Mar 28, 2024

jrhee17 commented Apr 2, 2024

jrhee17 left a comment

ikhoon left a comment

minwoox left a comment

	boolean equalsHostnamePattern(String hostnamePattern) {
	boolean equalsHostnamePattern(String validhostnamePattern, int port) {

Reuse VirtualHostBuilder on the same hostnamePattern #5418

Reuse VirtualHostBuilder on the same hostnamePattern #5418

Conversation

yejinio commented Jan 29, 2024

CLAassistant commented Jan 29, 2024 • edited Loading

github-actions bot commented Jan 30, 2024 • edited Loading

🔍 Build Scan® (commit: 33630fa)

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yejinio Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

minwoox Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yejinio Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 commented Mar 28, 2024

jrhee17 commented Apr 2, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

CLAassistant commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 30, 2024 •

edited

Loading

🔍 Build Scan® (commit: `33630fa`)

codecov bot commented Feb 7, 2024 •

edited

Loading

yejinio Feb 23, 2024 •

edited

Loading

minwoox Feb 23, 2024 •

edited

Loading

yejinio Mar 28, 2024 •

edited

Loading