Enhance Connection.isSame (#2128) #2180
Conversation
WalkthroughAdded detailed Javadoc to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
christiangoerdes
changed the title
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| public boolean isSame(String host, int port) { | ||
| if (socket == null || port != socket.getPort()) return false; | ||
| try { | ||
| InetAddress target = InetAddress.getByName(host); | ||
| InetAddress remote = ((InetSocketAddress) socket.getRemoteSocketAddress()).getAddress(); | ||
| return target.equals(remote) || (target.isLoopbackAddress() && remote.isLoopbackAddress()); | ||
| } catch (UnknownHostException e) { | ||
| return host.equals(this.host); | ||
| } |
There was a problem hiding this comment.
Do not short-circuit on socket.getPort() when a proxy is involved.
When this connection was established via an explicit HTTP proxy (see proxyConfiguration), the socket is connected to the proxy host/port while this.host still holds the original target. In that scenario isSame() is invoked with the target port (e.g. 80) but socket.getPort() returns the proxy port (e.g. 3128). The new early return therefore always yields false, preventing pooled proxy connections from ever being reused. Please amend the logic so that proxy-backed connections continue to match (e.g. only enforce the port check when no proxy is configured, or compare against the proxy port accordingly) before returning false.
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java
around lines 161 to 169, the method short-circuits by comparing the passed port
to socket.getPort() which breaks reuse when the socket is connected to an
explicit HTTP proxy (socket port = proxy port, while the method is called with
the original target port). Update the logic to account for proxyConfiguration:
only enforce the socket port check when no proxy is configured (or compare
against the proxy's configured port when proxyConfiguration is present), and
otherwise proceed to the host/address comparison so proxy-backed connections can
still match and be reused.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java (1)
161-170: Optional: make host comparison case-insensitive.Hostnames are case-insensitive. Consider
equalsIgnoreCaseto avoid surprising mismatches.- return host.equals(this.host) && port == this.port; + return host != null && host.equalsIgnoreCase(this.host) && port == this.port;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java(1 hunks)core/src/test/java/com/predic8/membrane/core/transport/http/ConnectionTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/test/java/com/predic8/membrane/core/transport/http/ConnectionTest.java (1)
55-56: LGTM: assertions now match exact host strings.These align with the current
isSamesemantics that compare host strings directly.Consider adding a test for the explicit HTTP proxy case (no tunnel) to ensure pooled connections remain reusable behind a proxy once
isSamecompares against the original target port.core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java (1)
161-170: Fix isSame for explicit HTTP proxy: compare against the original target port (not socket.getPort()).Plain HTTP via an explicit proxy connects the socket to the proxy port (e.g., 3128) while
isSame()is called with the origin port (e.g., 80). Comparing againstsocket.getPort()prevents pooled connections from being reused. Compare against the originally requested target port instead, and align the Javadoc.Apply this diff within this method and Javadoc:
- /** - * Checks whether the (host,port) combination matches this combination. - * - * This does not directly check whether the same proxy is used, but that is actually irrelevant, since - * proxyConfiguration is constant per HttpClient and ergo per ConnectionManager. Since only Connections - * opened by the same HttpClient are managed by the same ConnectionManager, this is sufficient. - */ - public boolean isSame(String host, int port) { - return socket != null && host.equals(this.host) && port == socket.getPort(); - } + /** + * Checks whether the (host, port) pair matches the original target of this connection. + * + * Note: When an explicit HTTP proxy is used without tunneling, the underlying socket is connected + * to the proxy. We therefore compare against the originally requested target port captured at + * connect-time, not {@code socket.getPort()}. + */ + public boolean isSame(String host, int port) { + if (socket == null) return false; + return host.equals(this.host) && port == this.port; + }Additional changes required outside this hunk (constructor and field):
// 1) Add a field to store the original target port (near the existing 'host' field): public final int port; // 2) Change the constructor to accept and store 'port': private Connection(ConnectionManager mgr, String host, int port, @Nullable SSLProvider sslProvider, @Nullable String sniServerName, @Nullable ProxyConfiguration proxy, SSLProvider proxySSLProvider) { this.mgr = mgr; this.host = host; this.port = port; this.sslProvider = sslProvider; this.sniServerName = sniServerName; this.proxyConfiguration = proxy; this.proxySSLProvider = proxySSLProvider; } // 3) Update the instantiation in open(...): // before: Connection con = new Connection(mgr, host, sslProvider, sniServername, proxy, proxySSLProvider); // after (note: at this point 'host' and 'port' are still the original target values): Connection con = new Connection(mgr, host, port, sslProvider, sniServername, proxy, proxySSLProvider);Run to find and update all constructor call sites (should be only one, but confirm), and to review all usages of isSame:
#!/bin/bash set -euo pipefail echo "Constructor call sites:" rg -nP -C2 '(?<![A-Za-z0-9_])new\s+Connection\s*\(' echo echo "isSame call sites (context):" rg -nP -C3 '(?<![A-Za-z0-9_])isSame\s*\('
2 participants
Summary by CodeRabbit
(Note: No functional changes; behavior remains the same.)