Add VirtualHost support for random ports (#6410)#6603
Add VirtualHost support for random ports (#6410)#6603junjunclub wants to merge 6 commits intoline:mainfrom
Conversation
junjunclub
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ServerPort-aware virtual hosts and runtime port mapping: ServerPort now tracks actual bound ports; ServerBuilder gains Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SB as ServerBuilder
participant SP as ServerPort
participant VHB as VirtualHostBuilder
participant S as Server
participant DSC as DefaultServerConfig
participant Client as Client
App->>SB: port(ServerPort) / port(0)
SB->>SP: create ServerPort(ephemeral)
App->>SB: virtualHost(ServerPort)
SB->>VHB: new VirtualHostBuilder(ServerPort)
VHB-->>SB: register VirtualHostBuilder(serverPort)
App->>SB: build()
SB->>S: build/start
S->>SP: bind to actual port (OS)
S->>SP: setActualPort(actualPort)
S->>DSC: rebuildDomainAndPortMapping()
Client->>DSC: findVirtualHost(hostname, actualPort)
DSC-->>Client: return VirtualHost bound to original ServerPort
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java`:
- Around line 2506-2513: The validation currently uses
this.ports.contains(serverPort) which relies on equals() but runtime matching
requires reference identity; update the check in the ServerBuilder validation
loop (involving VirtualHostBuilder and its serverPort() call) to test identity
instead of equality (e.g., ensure any entry in this.ports is == serverPort) and
keep the existing checkState message; locate the block using VirtualHostBuilder
vhb, serverPort(), and this.ports and replace the contains(...) condition with
an identity-based predicate (anyMatch(p -> p == serverPort) or equivalent).
- Around line 1860-1889: The new public overload virtualHost(ServerPort) in
ServerBuilder must be annotated with `@UnstableApi` to mark the API as unstable;
update the declaration of the virtualHost(ServerPort) method in ServerBuilder to
add the `@UnstableApi` annotation (importing it if needed) so the public API
surface for VirtualHostBuilder returned by virtualHost(ServerPort) is properly
marked per project guidelines.
🧹 Nitpick comments (2)
core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java (1)
241-331: Add assertions for/barrouting invirtualHostWithServerPort.
Right now/barresponses are computed but never asserted, so misrouting could slip through.✅ Proposed test assertion
// Check /bar on both ports final AggregatedHttpResponse resp1Bar = client1.get("/bar").aggregate().join(); final AggregatedHttpResponse resp2Bar = client2.get("/bar").aggregate().join(); // One port should serve /foo with OK, the other with NOT_FOUND final boolean port1ServesFoo = resp1Foo.status() == HttpStatus.OK; final boolean port2ServesFoo = resp2Foo.status() == HttpStatus.OK; assertThat(port1ServesFoo || port2ServesFoo) .as("At least one port should serve /foo, but got: port1=%s, port2=%s", resp1Foo.status(), resp2Foo.status()) .isTrue(); + // One port should serve /bar with OK as well. + final boolean port1ServesBar = resp1Bar.status() == HttpStatus.OK; + final boolean port2ServesBar = resp2Bar.status() == HttpStatus.OK; + assertThat(port1ServesBar || port2ServesBar) + .as("At least one port should serve /bar, but got: port1=%s, port2=%s", + resp1Bar.status(), resp2Bar.status()) + .isTrue();core/src/main/java/com/linecorp/armeria/server/VirtualHost.java (1)
316-341: Consider documenting the dynamic port resolution behavior.The
defaultHostname()andhostnamePattern()methods now return different values depending on whether the server has bound to its ports. Before binding,serverPort.localAddress()returnsnull, so the pre-computed value is returned. After binding, the actual ephemeral port is appended.This dynamic behavior is intentional for ephemeral port support, but callers may not expect the return value to change over the VirtualHost lifecycle. Consider adding Javadoc clarification, e.g.:
/** * Returns the default hostname of this virtual host. If this virtual host is bound to a * {`@link` ServerPort} with an ephemeral port (0), the actual bound port is appended after * the server starts. */
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
Outdated
Show resolved
Hide resolved
| return buf.toString(); | ||
| } | ||
| } | ||
| } No newline at end of file |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java`:
- Around line 241-286: The test virtualHostWithServerPort collects responses for
/foo and /bar but never asserts the /bar results; update the test to compute
booleans for resp1Bar.status() and resp2Bar.status() (e.g., port1ServesBar,
port2ServesBar) and add assertions: (1) at least one port serves /bar
(port1ServesBar || port2ServesBar) and (2) ensure exclusivity per port by
asserting each port serves exactly one of the endpoints (for each port assert
(servesFoo ^ servesBar) is true), referencing the existing
resp1Foo/resp2Foo/resp1Bar/resp2Bar variables in virtualHostWithServerPort so
the test verifies both routing and exclusivity.
♻️ Duplicate comments (2)
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (2)
1860-1903: Add@UnstableApito the new public overload.
New public APIs should be annotated as unstable per project guidelines.As per coding guidelines, ...✅ Proposed fix
+ `@UnstableApi` public VirtualHostBuilder virtualHost(ServerPort serverPort) { requireNonNull(serverPort, "serverPort");
2506-2513: ValidateServerPortby identity, not equals.
contains()usesequals, but runtime matching uses reference identity (==), so a different-but-equal instance would pass validation yet never match.🐛 Proposed fix
- if (serverPort != null) { - checkState(this.ports.contains(serverPort), + if (serverPort != null) { + final boolean presentByIdentity = this.ports.stream().anyMatch(p -> p == serverPort); + checkState(presentByIdentity, "The ServerPort for a virtual host is not in the server's port list. " + "Please add the ServerPort using port(ServerPort) before creating a virtual host."); }
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java (1)
427-445: Consider avoiding per-request full scans when no ServerPort-based vhosts exist.
findVirtualHost(...)now iterates active ports (and then virtual hosts) on every request; a boolean guard or a precomputed map could keep the hot path cheaper.
core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6603 +/- ##
============================================
- Coverage 74.46% 74.42% -0.05%
- Complexity 22234 23922 +1688
============================================
Files 1963 2146 +183
Lines 82437 89199 +6762
Branches 10764 11688 +924
============================================
+ Hits 61385 66384 +4999
- Misses 15918 17243 +1325
- Partials 5134 5572 +438 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| final ServerPort originalPort = activePort.originalServerPort(); | ||
| if (originalPort != null) { | ||
| // Find the VirtualHost that has this original ServerPort | ||
| for (VirtualHost vh : virtualHosts) { |
There was a problem hiding this comment.
Question) Is it possible to optimize this?
Personally, I think it is fine if a map of (ephemeral ports -> virtual hosts) is created right after all ports are bound (or each port is bound), and right before ServerStartStopSupport's future is completed.
At the very least, given that it is fairly common for multiple ports/virtual hosts to be specified, I prefer the common path not to iterate through all hosts/ports. This is especially since this is a critical path called for every request.
There was a problem hiding this comment.
You're right. My initial focus was on making the feature work correctly with minimal changes,
so I used a simple iteration approach. However, I agree that O(n * m) iteration on every request
is inefficient for this critical path.
I’m planning to use a volatile field with an immutable map for this, as it seems suitable for a write-once, read-many scenario. Would you consider this a sound approach, or do you see any potential pitfalls I might have overlooked?
There was a problem hiding this comment.
I’m planning to use a volatile field with an immutable map for this, as it seems suitable for a write-once, read-many scenario. Would you consider this a sound approach, or do you see any potential pitfalls I might have overlooked?
Sure, the approach sounds reasonable to me
There was a problem hiding this comment.
Using random ports for port-based virtual hosting seems more complex than I initially thought. It may not be a good-first-issue so I suggest the following approach as guidance.
- Use
ServerPortas the key forvirtualHostAndPortMapping.
private final Map<ServerPort, Mapping<String, VirtualHost>> virtualHostAndPortMapping; - Store the
ServerPortset viaServerBuilderinServerChannelattributes.
armeria/core/src/main/java/com/linecorp/armeria/server/Server.java
Lines 783 to 793 in d6103ba
- When handling a request, get the
ServerPortfrom the parent channel and look up theVirtualHostviafindVirtualHost(String, ServerPort).
Since I haven't implemented a POC, this proposal may not work as intended or could have unexpected issues.
There was a problem hiding this comment.
Store the ServerPort set via ServerBuilder in ServerChannel attributes
If this approach is taken, we should also consider server reconfiguration. (or at least decide whether this will be supported for server reconfiguration)
There was a problem hiding this comment.
If this approach is taken, we should also consider server reconfiguration. (or at least decide whether this will be supported for server reconfiguration)
Good point. Do you know a technical limitation the prevents the original ServerPorts from being used for reconfiguration? Otherwise, should we use the original ones?
There was a problem hiding this comment.
If this approach is taken, we should also consider server reconfiguration. (or at least decide whether this will be supported for server reconfiguration)
I think I may have misunderstood the original proposal by @ikhoon . Storing ServerPort into the channel attribute and querying the VirtualHost sounds like a good approach to me 👍
There was a problem hiding this comment.
On second thought, we could allow ServerPort to update the actual port once when the server obtains an ephemeral port. In that case, storing ServerPort into the channel would be unnecessary.
ServerPort serverPort = ...;
// This is allowed when the port is 0 and `actualPort` hasn't been set yet.
serverPort.actualPort(...)There was a problem hiding this comment.
Thank you for the suggestion!
I'll implement the actualPort approach. I plan to add an actualPort field to ServerPort that can only be set once when the port is 0. After successful port binding, Server will call port.actualPort(boundPort) to store the assigned ephemeral port. Then findVirtualHost will match requests using the serverPort.actualPort() value. This way, storing ServerPort in channel attributes becomes unnecessary and the design is simpler.
Does this approach look correct?
There was a problem hiding this comment.
Yes, that’s what I was thinking.
| assertThat(actualPort1).isGreaterThan(0); | ||
| assertThat(actualPort2).isGreaterThan(0); | ||
| assertThat(actualPort1).isNotEqualTo(actualPort2); | ||
|
|
There was a problem hiding this comment.
Given that the hostnamePattern is used extensively, do you mind also verifying hostnamePattern is set correctly here? (It would also help users if VirtualHost#port also returns the resolved port so that users can be aware of which virtual host is served by which port)
There was a problem hiding this comment.
Thanks for the feedback! I'll add assertions to verify that the hostnamePattern is set correctly.
Regarding VirtualHost#port() returning the resolved port, I agree this is necessary and will include the change to ensure the feature is fully integrated. I'll make sure to have this updated along with the assertions.
There was a problem hiding this comment.
It would also help users if
VirtualHost#portalso returns the resolved port so that users can be aware of which virtual host is served by which port
Thank you for the feedback regarding VirtualHost#port().
Currently, port() returns 0 for ephemeral ports because it only returns the final int port assigned at build time. I noticed that hostnamePattern() already handles this by referencing serverPort.localAddress() to return the resolved port.
I propose updating port() to follow the same pattern:
VirtualHost.java
public int port() {
if (serverPort != null && serverPort.localAddress() != null) {
final int actualPort = serverPort.localAddress().getPort();
if (actualPort > 0) {
return actualPort;
}
}
return port;
}This change allows the test to verify the exact resolved port using a more descriptive assertion.
Updated Test Case:
final VirtualHost vh = server.config().virtualHosts().stream()
.filter(v -> v.serverPort() == port1)
.findFirst()
.orElseThrow(() -> new NoSuchElementException("VirtualHost for port1 not found"));
assertThat(vh.port())
.as("VirtualHost bound to %s should have resolved port %d", port1, actualPort1)
.isEqualTo(actualPort1);Does this approach look reasonable to you, or is there a more preferred way in Armeria to handle this? I'm also open to any other suggestions if you think there's a better architectural direction for resolving ports in VirtualHost. If you agree, I'm happy to include this enhancement in the current PR.
Thank you!
There was a problem hiding this comment.
@jrhee17
While I was drafting my response, I noticed a new review has been submitted. I will carefully review the feedback and update the PR accordingly. Thank you for the guidance!
…lookup - Add actualPort field and methods to ServerPort for storing actual bound port - Add actualPortToVirtualHost map to DefaultServerConfig for O(1) lookup - Update Server to set actualPort after binding and build port mapping - Add tests for actualPort and multiple random port VirtualHosts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/linecorp/armeria/server/Server.java (1)
800-825:⚠️ Potential issue | 🔴 CriticalAdd build-time validation to prevent
virtualHost(ServerPort)with port groups or ensuresetActualPort()is called for port group reuse scenarios.When a
ServerPortbelongs to a port group and reuses a previously bound port inNextServerPortStartListener(lines 900-904), the temporaryServerPortcreated doesn't havesetActualPort()called on the original config port. This causesbuildActualPortMapping()to skip creating routing entries for ephemeral ports, breaking port-based virtual host routing silently.Consider adding a build-time check in
ServerBuilderto either:
- Reject configurations where
virtualHost(ServerPort)uses aServerPortwith aportGroup, or- Fix the runtime behavior to ensure
setActualPort()is called for all reused ports in a port group
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java`:
- Around line 405-423: ServerBuilder.virtualHost(ServerPort) currently accepts
any ServerPort but DefaultServerConfig.buildActualPortMapping only maps
ephemeral ports (where ServerPort.localAddress().getPort() == 0), so add a
validation in ServerBuilder.virtualHost(ServerPort) to enforce ephemeral ports:
check serverPort.localAddress().getPort() == 0 and throw an
IllegalArgumentException (or use Objects.requireNonNull/assert) with a clear
message if not ephemeral; reference ServerBuilder.virtualHost(ServerPort),
ServerPort.localAddress().getPort(), and
DefaultServerConfig.buildActualPortMapping to locate the related logic.
In `@core/src/main/java/com/linecorp/armeria/server/ServerPort.java`:
- Around line 249-260: Add the `@UnstableApi` annotation to the new public method
actualPort() in the ServerPort class to follow project guidelines for newly
added public APIs; update imports to include the UnstableApi annotation (e.g.,
com.linecorp.armeria.common.annotation.UnstableApi) and place the annotation
immediately above the actualPort() method signature so the method is marked as
unstable.
🧹 Nitpick comments (2)
core/src/main/java/com/linecorp/armeria/server/ServerPort.java (2)
270-277:setActualPorthas a non-atomic check-then-act on a volatile field.The guard
checkState(this.actualPort == -1, ...)followed bythis.actualPort = actualPortis not atomic. While this is package-private and currently only called from a single event-loop thread inServerPortStartListener, a future caller from a different thread could trigger a race where two threads both see-1and both proceed to write.Consider either:
- Documenting the single-thread assumption (e.g.,
@GuardedByor a Javadoc note), or- Using a CAS via
AtomicInteger/VarHandlefor correctness.
337-345:toString()does not reflectactualPortororiginalServerPort.After binding,
actualPortholds meaningful diagnostic information (the real ephemeral port). Including it intoString()would improve debuggability when inspecting bound ports in logs.
| /** | ||
| * Builds the O(1) lookup map from actual bound ports to VirtualHosts. | ||
| * This should be called after all ports are bound. | ||
| */ | ||
| void buildActualPortMapping() { | ||
| final Int2ObjectMap<VirtualHost> mapping = new Int2ObjectOpenHashMap<>(); | ||
| for (VirtualHost vh : virtualHosts) { | ||
| final ServerPort sp = vh.serverPort(); | ||
| if (sp != null && sp.localAddress().getPort() == 0) { | ||
| final int actualPort = sp.actualPort(); | ||
| if (actualPort > 0) { | ||
| mapping.put(actualPort, vh); | ||
| } | ||
| } | ||
| } | ||
| if (!mapping.isEmpty()) { | ||
| actualPortToVirtualHost = mapping; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ServerBuilder and virtualHost method implementations
find . -name "ServerBuilder.java" -type f | head -5Repository: line/armeria
Length of output: 123
🏁 Script executed:
# Search for virtualHost method with ServerPort parameter
rg -n "virtualHost.*ServerPort" --type=java -B2 -A10 | head -100Repository: line/armeria
Length of output: 8958
🏁 Script executed:
# Search for ServerPort class definition
find . -name "ServerPort.java" -type fRepository: line/armeria
Length of output: 120
🏁 Script executed:
# Look for port validation in ServerBuilder context
rg -n "ServerPort.*port.*0|ephemeral" --type=java -B2 -A2 | head -80Repository: line/armeria
Length of output: 8867
🏁 Script executed:
# Check ServerPort implementation to understand the actual port handling
cat core/src/main/java/com/linecorp/armeria/server/ServerPort.javaRepository: line/armeria
Length of output: 12854
🏁 Script executed:
# Look at buildDomainMapping to understand the full context
sed -n '330,350p' core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javaRepository: line/armeria
Length of output: 1158
🏁 Script executed:
# Check if there's any validation in ServerBuilder that would prevent non-ephemeral ServerPort
sed -n '1880,1920p' core/src/main/java/com/linecorp/armeria/server/ServerBuilder.javaRepository: line/armeria
Length of output: 1940
🏁 Script executed:
# Look for any documentation or comments about ServerPort usage constraints
rg -n "ServerPort.*ephemeral|virtualHost.*ServerPort" --type=java --type=mdx | grep -i "comment\|javadoc\|ephemeral\|doc" | head -20Repository: line/armeria
Length of output: 83
🏁 Script executed:
# Check if there's any validation added in ServerBuilder for ephemeral ports
rg -n "virtualHost.*ServerPort|port.*0|ephemeral" core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java -B3 -A3 | head -60Repository: line/armeria
Length of output: 2365
🏁 Script executed:
# Check if buildActualPortMapping is tested with non-ephemeral ports
rg -n "buildActualPortMapping|ServerPort.*[89][0-9]{2,}" core/src/test/java/com/linecorp/armeria/server/ -B2 -A2 | head -80Repository: line/armeria
Length of output: 1843
🏁 Script executed:
# Check what happens after buildActualPortMapping - how is actualPortToVirtualHost used?
sed -n '450,470p' core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javaRepository: line/armeria
Length of output: 907
🏁 Script executed:
# Look for any VirtualHostBuilder validation that might restrict ServerPort to ephemeral
rg -n "class VirtualHostBuilder" core/src/main/java/com/linecorp/armeria/server/ -A 30 | head -80Repository: line/armeria
Length of output: 3644
🏁 Script executed:
# Check if there are any test cases that try to use non-ephemeral ServerPort with virtualHost()
rg -n "virtualHost.*new ServerPort.*[89]" core/src/test/java/com/linecorp/armeria/server/ -B2 -A5Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check the VirtualHostBuilder constructor to see if it validates ServerPort
sed -n '145,180p' core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.javaRepository: line/armeria
Length of output: 1332
🏁 Script executed:
# Look for the build() method of VirtualHostBuilder to see if it creates VirtualHost with validation
rg -n "VirtualHostBuilder.*build\(\)|new VirtualHost" core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java -A 5 | head -60Repository: line/armeria
Length of output: 2308
🏁 Script executed:
# Get the full build() method from VirtualHostBuilder
sed -n '1400,1530p' core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.javaRepository: line/armeria
Length of output: 7353
🏁 Script executed:
# Check if there's validation for ephemeral port requirement for ServerPort-based VirtualHosts
rg -n "serverPort.*port.*0|ephemeral.*ServerPort|build\(\)" core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java -B3 -A3 | head -100Repository: line/armeria
Length of output: 3314
🏁 Script executed:
# Check the VirtualHost constructor to see if it validates serverPort
rg -n "class VirtualHost|VirtualHost(" core/src/main/java/com/linecorp/armeria/server/VirtualHost.java -A 2 | head -50Repository: line/armeria
Length of output: 166
🏁 Script executed:
# Check if there are any integration tests that would catch the unreachable VirtualHost issue
rg -n "virtualHost.*ServerPort.*8080|virtualHost.*new ServerPort.*[1-9]" core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java -B2 -A5Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check VirtualHost constructor
rg -n "public VirtualHost\(" core/src/main/java/com/linecorp/armeria/server/VirtualHost.java -A 30 | head -80Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for any validation logic in VirtualHost that checks serverPort
rg -n "serverPort.*port.*0|checkArgument.*serverPort|checkState.*serverPort" core/src/main/java/com/linecorp/armeria/server/VirtualHost.java -B2 -A2Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check the actual port in the comment about ServerPort-based virtual hosts
sed -n '1860,1900p' core/src/main/java/com/linecorp/armeria/server/ServerBuilder.javaRepository: line/armeria
Length of output: 1793
ServerBuilder.virtualHost(ServerPort) must validate that the port is ephemeral (port 0).
The Javadoc and design explicitly state this API is for "random port (port 0)", and buildActualPortMapping only handles ephemeral ports where sp.localAddress().getPort() == 0. However, the API accepts any ServerPort without validation. A user calling virtualHost(new ServerPort(8080, HTTP)) would create a silently unreachable VirtualHost—it would be skipped by both the domain mapping (line 338) and the actual port mapping (requires port == 0).
Add a port validation check in ServerBuilder.virtualHost(ServerPort) to enforce that serverPort.localAddress().getPort() == 0, or document this constraint with a runtime assertion.
🤖 Prompt for AI Agents
In `@core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java`
around lines 405 - 423, ServerBuilder.virtualHost(ServerPort) currently accepts
any ServerPort but DefaultServerConfig.buildActualPortMapping only maps
ephemeral ports (where ServerPort.localAddress().getPort() == 0), so add a
validation in ServerBuilder.virtualHost(ServerPort) to enforce ephemeral ports:
check serverPort.localAddress().getPort() == 0 and throw an
IllegalArgumentException (or use Objects.requireNonNull/assert) with a clear
message if not ephemeral; reference ServerBuilder.virtualHost(ServerPort),
ServerPort.localAddress().getPort(), and
DefaultServerConfig.buildActualPortMapping to locate the related logic.
- Add @UnstableApi annotation to ServerPort.actualPort() per Armeria conventions - Add fixedPortVirtualHostWithServerPort() test to verify virtualHost(ServerPort) works correctly
| } | ||
| } | ||
| if (!mapping.isEmpty()) { | ||
| actualPortToVirtualHost = mapping; |
There was a problem hiding this comment.
Should we defer building domain and port mapping until the actual port is resolved instead of adding a new maping?
There was a problem hiding this comment.
That's a good point. Deferring virtualHostAndPortMapping until after ports are bound would let us unify the two maps into one. The tradeoff is that virtualHostAndPortMapping is currently final and would need to become volatile, and the buildDomainAndPortMapping() logic would need to incorporate actual port resolution. I'm happy to refactor if you prefer a unified approach.
| * or {@code null} if this is an original port. | ||
| */ | ||
| @Nullable | ||
| ServerPort originalServerPort() { |
There was a problem hiding this comment.
Question) Is this only used for testing?
There was a problem hiding this comment.
Yes, with the actualPortToVirtualHost map replacing the old loop in findVirtualHost(), originalServerPort() is no longer used in production code only in tests. I can remove it if it's no longer needed.
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Changes look reasonable to me 👍
| if (serverPort != null && serverPort.localAddress() != null) { | ||
| final int actualPort = serverPort.localAddress().getPort(); | ||
| if (actualPort > 0) { | ||
| return originalDefaultHostname + ':' + actualPort; |
There was a problem hiding this comment.
nit; could this be cached? Depending on the call path, this may be called for every request. Ditto for hostnamePattern.
There was a problem hiding this comment.
Thank you for the suggestion! Since actualPort is fixed after server start, caching the computed string would avoid unnecessary allocation on repeated calls. I'll add cached fields and compute them lazily on the first access after the port is bound. 👍👍
| * Returns the name of the default host. | ||
| */ | ||
| public String defaultHostname() { | ||
| if (serverPort != null && serverPort.localAddress() != null) { |
There was a problem hiding this comment.
Question) Can localAddress be null?
| if (serverPort != null && serverPort.localAddress() != null) { | |
| if (serverPort != null) { |
There was a problem hiding this comment.
Thank you for catching this. You're right, localAddress() returns a final field that is initialized with requireNonNull in the constructor, so it can never be null. I'll remove the unnecessary null check.
- Defer VirtualHost domain/port mapping until after ports are bound, unifying virtualHostAndPortMapping and actualPortToVirtualHost into a single volatile map that is rebuilt via rebuildDomainAndPortMapping() - Remove originalServerPort from ServerPort as it is no longer used after the mapping unification - Cache defaultHostname() and hostnamePattern() results in VirtualHost to avoid repeated String allocation on every call - Remove unnecessary null check on ServerPort.localAddress() which is guaranteed non-null by requireNonNull in constructor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java (2)
447-465:⚠️ Potential issue | 🟠 MajorMultiple reads of volatile
virtualHostAndPortMapping— use a local snapshot.
findVirtualHostreads the volatile field on lines 449, 453, and 463 without capturing it in a local variable. IfrebuildDomainAndPortMapping()replaces the reference between reads, different reads may see different map instances, leading to inconsistent lookups within a single call.Proposed fix
`@Override` public VirtualHost findVirtualHost(String hostname, int port) { - if (virtualHostAndPortMapping == null) { + final Int2ObjectMap<Mapping<String, VirtualHost>> mapping = virtualHostAndPortMapping; + if (mapping == null) { return defaultVirtualHost; } - final Mapping<String, VirtualHost> virtualHostMapping = virtualHostAndPortMapping.get(port); + final Mapping<String, VirtualHost> virtualHostMapping = mapping.get(port); if (virtualHostMapping != null) { final VirtualHost virtualHost = virtualHostMapping.map(hostname + ':' + port); // Exclude the default virtual host from port-based virtual hosts. if (virtualHost != defaultVirtualHost) { return virtualHost; } } // No port-based virtual host is configured. Look for named-based virtual host. - final Mapping<String, VirtualHost> nameBasedMapping = requireNonNull(virtualHostAndPortMapping.get(-1)); + final Mapping<String, VirtualHost> nameBasedMapping = requireNonNull(mapping.get(-1)); return nameBasedMapping.map(hostname); }
439-445:⚠️ Potential issue | 🟡 MinorSame volatile read issue in deprecated
findVirtualHost(String).This method also reads
virtualHostAndPortMappingtwice without a local snapshot (lines 440 and 443). Apply the same local-variable pattern for consistency.Proposed fix
`@Override` `@Deprecated` public VirtualHost findVirtualHost(String hostname) { - if (virtualHostAndPortMapping == null) { + final Int2ObjectMap<Mapping<String, VirtualHost>> mapping = virtualHostAndPortMapping; + if (mapping == null) { return defaultVirtualHost; } - final Mapping<String, VirtualHost> virtualHostMapping = virtualHostAndPortMapping.get(-1); + final Mapping<String, VirtualHost> virtualHostMapping = mapping.get(-1); return virtualHostMapping.map(hostname); }
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java (1)
241-293: Consider usingServerPort.actualPort()directly for clarity.The
serverPortBasedVirtualHostRoutesCorrectlyandmultipleRandomPortVirtualHoststests accessport1.actualPort()directly, which is cleaner than sortingactivePorts(). This test could be simplified similarly, though it still works correctly as-is.
| private final Function<RoutingContext, RequestId> requestIdGenerator; | ||
|
|
||
| VirtualHost(String defaultHostname, String hostnamePattern, int port, | ||
| @Nullable ServerPort serverPort, |
There was a problem hiding this comment.
This change seems to introduce a compilation error in RoutersBenchmark.
| final Map<Integer, VirtualHost> portMappingDefaultVhosts = | ||
| portMappingVhosts.stream() | ||
| .filter(v -> v.hostnamePattern().startsWith("*:")) | ||
| .collect(toImmutableMap(VirtualHost::port, Function.identity())); |
There was a problem hiding this comment.
Why was the motivation for this refactor? It seems to cause a regression. A unknown host no longer fall back to the default port-based virtual host. The following test passes in the main branch but fails with this PR.
diff --git core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java
index c759fe6b2..1bd47c390 100644
--- core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java
+++ core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java
@@ -60,13 +60,16 @@ class PortBasedVirtualHostTest {
sb.http(normalServerPort)
.http(virtualHostPort)
.http(fooHostPort)
+ .virtualHost("foo.com:" + fooHostPort)
+ .service("/foo", (ctx, req) -> HttpResponse.of("foo with port"))
+ .and()
+ .virtualHost(fooHostPort)
+ .service("/foo-default", (ctx, req) -> HttpResponse.of("foo with default host"))
+ .and()
.service("/normal", (ctx, req) -> HttpResponse.of("normal"))
.virtualHost(virtualHostPort)
.service("/managed", (ctx, req) -> HttpResponse.of("managed"))
.and()
- .virtualHost("foo.com:" + fooHostPort)
- .service("/foo", (ctx, req) -> HttpResponse.of("foo with port"))
- .and()
.virtualHost("foo.com")
.service("/foo-no-port", (ctx, req) -> HttpResponse.of("foo without port"))
.and()
@@ -154,6 +157,37 @@ class PortBasedVirtualHostTest {
}
}
+ @Test
+ void shouldFallbackToDefaultPortVirtualHost() {
+ try (ClientFactory factory = ClientFactory.builder()
+ .addressResolverGroupFactory(
+ unused -> MockAddressResolverGroup.localhost())
+ .build()) {
+
+ final WebClient client = WebClient.builder("http://foo.com:" + fooHostPort)
+ .factory(factory)
+ .build();
+ AggregatedHttpResponse response = client.get("/normal").aggregate().join();
+ // Fallback to default virtual host
+ assertThat(response.contentUtf8()).isEqualTo("normal");
+
+ response = client.get("/managed").aggregate().join();
+ assertThat(response.status()).isEqualTo(HttpStatus.NOT_FOUND);
+
+ response = client.get("/foo").aggregate().join();
+ assertThat(response.status()).isEqualTo(HttpStatus.OK);
+
+ response = client.get("/foo-no-port").aggregate().join();
+ assertThat(response.status()).isEqualTo(HttpStatus.NOT_FOUND);
+
+ final WebClient barClient = WebClient.builder("http://bar.com:" + fooHostPort)
+ .factory(factory)
+ .build();
+ response = barClient.get("/foo-default").aggregate().join();
+ assertThat(response.contentUtf8()).isEqualTo("foo with default host");
+ }
+ }
+
@Test
void zeroVirtualHostPort() {
assertThatThrownBy(() -> Server.builder().virtualHost(0))4 participants
Motivation
Currently, using
virtualHost(0)throws anIllegalArgumentException:java.lang.IllegalArgumentException: port: 0 (expected: 1-65535)This is problematic for CI environments where random ports are essential to avoid port conflicts. The existing
virtualHost(int port)API cannot support random ports because:intvalue0cannot distinguish between multiple random ports.virtualHost(0)twice returns the sameVirtualHostBuilderdue to the builder reuse logic.See: issue #6410
Modifications
ServerBuilder.virtualHost(ServerPort)API that uses reference equality to distinguish different random port configurations.VirtualHostBuilder.serverPortfield and constructor to supportServerPort-based virtual hosts.VirtualHost.serverPortfield and accessor.ServerPort.originalServerPortfield to track the original configuration when binding to ephemeral ports.ServerPortreference when creatingactualPortinServer.java.DefaultServerConfig.findVirtualHost()to route requests based on the actual bound port (priority lookup).VirtualHostBuilder.serverPort(),VirtualHost.serverPort(), andServerPort.originalServerPort()accessors.virtualHost(ServerPort)API routing behavior.Result
VirtualHosts to random ports usingServerPortobjects:Each
ServerPortinstance is assigned a different random port by the OS, and requests are correctly routed to the correspondingVirtualHost.Follow-up: O(1)
ServerPort-based VirtualHost lookup (new in this PR)While implementing
virtualHost(ServerPort), we noticed thatfindVirtualHost(String hostname, int port)can become inefficient forServerPort-based virtual hosts using ephemeral ports, because it may scanactivePortsandvirtualHoststo resolve the actual bound port.This PR also introduces an
actualPort()approach and a precomputed mapping to make this routing step O(1) after all ports are bound.Additional changes
ServerPort.actualPort()volatile int actualPortto store the actual bound port.actualPort()getter and package-privatesetActualPort(int)setter.Server.javaport.setActualPort(...)after binding ephemeral ports.DefaultServerConfigvolatile Map<Integer, VirtualHost> actualPortToVirtualHost.buildActualPortMapping()to populate the mapping after all ports are bound.findVirtualHost(String, int)to prefer the O(1) map lookup forServerPort-based routing.Tests
ServerPort.actualPort()getter/setter behavior.actualPortis reflected after server start and that multiple random-port virtual hosts work as expected.Outcome
findVirtualHost()now performs O(1) lookup forServerPort-based virtual host routing once ports are bound.ServerPort.actualPort()provides a clean API to retrieve the actual bound port.