Simplify SessionProtocolNegotiationCache key generation

line · trustin · Jun 8, 2023 · May 19, 2023 · May 20, 2023 · May 20, 2023
commit a5aa7fe76d42595f33d4776a1a7605589e666aec
@@ -19,10 +19,8 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static java.util.Objects.requireNonNull;
 
-import java.io.UnsupportedEncodingException;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
-import java.net.URLDecoder;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
@@ -37,6 +35,7 @@
 import com.linecorp.armeria.common.SessionProtocol;
 import com.linecorp.armeria.common.util.DomainSocketAddress;
 import com.linecorp.armeria.common.util.LruMap;
+import com.linecorp.armeria.internal.common.util.DomainSocketPathEscaper;
 
 /**
  * Keeps the recent {@link SessionProtocol} negotiation failures. It is an LRU cache which keeps at most
@@ -158,55 +157,41 @@ private static CacheEntry getOrCreate(String key) {
 
     @VisibleForTesting
     static String key(Endpoint endpoint) {
-        final String key;
-        if (endpoint.isDomainSocket()) {
-            try {
-                key = URLDecoder.decode(endpoint.host(), "UTF-8");
-            } catch (UnsupportedEncodingException e) {
-                throw new Error(e);
-            }
-        } else {
-            checkArgument(endpoint.hasPort(), "endpoint must have a port.");
-            // It's okay to create the key using endpoint.host():
-            // - If the endpoint has an IP address without host name, the IP address is used in the key to store
-            //   and retrieve the value.
-            // - If the endpoint has an IP address with host name, the host name is used in the key to store
-            //   and retrieve the value.
-            // - If the endpoint has host name only, the host name is used in the key to store and retrieve
-            //   the value.
-            key = key(endpoint.host(), endpoint.port());
-        }
-        return key;
+        checkArgument(endpoint.hasPort() || endpoint.isDomainSocket(),
+                      "endpoint: %s (expected: has a port or is a domain socket address)", endpoint);
+
+        // It's okay to create the key using endpoint.authority():
+        // - If the endpoint has an IP address without host name, the IP address is used in the key to store
+        //   and retrieve the value.
+        // - If the endpoint has an IP address with host name, the host name is used in the key to store
+        //   and retrieve the value.
+        // - If the endpoint has host name only, the host name is used in the key to store and retrieve
+        //   the value.
+        // - If the endpoint is a domain socket address, the encoded path is used in the key to store and
+        //   retrieve the value.
+        return endpoint.authority();
     }
 
     @VisibleForTesting
     static String key(SocketAddress remoteAddress) {
         requireNonNull(remoteAddress, "remoteAddress");
         if (remoteAddress instanceof InetSocketAddress) {
             if (remoteAddress instanceof DomainSocketAddress) {
-                return "unix:" + ((DomainSocketAddress) remoteAddress).path();
+                return ((DomainSocketAddress) remoteAddress).authority();
             } else {
                 final InetSocketAddress raddr = (InetSocketAddress) remoteAddress;
-                return key(raddr.getHostString(), raddr.getPort());
+                return raddr.getHostString() + ':' + raddr.getPort();
             }
         }
 
         if (remoteAddress instanceof io.netty.channel.unix.DomainSocketAddress) {
-            return "unix:" + ((io.netty.channel.unix.DomainSocketAddress) remoteAddress).path();
+            return DomainSocketPathEscaper.toAuthority(
+                    ((io.netty.channel.unix.DomainSocketAddress) remoteAddress).path());
         }
 
         throw new IllegalArgumentException("unsupported remote address: " + remoteAddress);
     }
 
-    @VisibleForTesting
-    static String key(String host, int port) {
-        return new StringBuilder(host.length() + 6)
-                .append(host)
-                .append(':')
-                .append(port)
-                .toString();
-    }
-
     private static long convertToWriteLock(long stamp) {
         final long writeStamp = lock.tryConvertToWriteLock(stamp);
         if (writeStamp == 0L) {

@@ -24,16 +24,11 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.net.UrlEscapers;
 
 import com.linecorp.armeria.client.Endpoint;
 import com.linecorp.armeria.common.annotation.Nullable;
 import com.linecorp.armeria.common.annotation.UnstableApi;
-import com.linecorp.armeria.internal.common.util.TemporaryThreadLocals;
+import com.linecorp.armeria.internal.common.util.DomainSocketPathEscaper;
 
 /**
  * An {@link InetSocketAddress} that refers to the {@link Path} of a Unix domain socket.
@@ -125,7 +120,7 @@ public String authority() {
             return authority;
         }
 
-        final String newAuthority = escape("unix:" + path);
+        final String newAuthority = DomainSocketPathEscaper.toAuthority(path.toString());
         this.authority = newAuthority;
         return newAuthority;
     }
@@ -168,27 +163,4 @@ public Endpoint asEndpoint() {
     public String toString() {
         return path.toString();
     }
-
-    private static final Pattern AT_OR_COLON = Pattern.compile("[@:]");
-
-    @VisibleForTesting
-    static String escape(String authority) {
-        final String escaped = UrlEscapers.urlPathSegmentEscaper().escape(authority);
-        // We need to escape `@` and `:` as well, so that the authority contains neither userinfo nor port.
-        final Matcher matcher = AT_OR_COLON.matcher(escaped);
-        try (TemporaryThreadLocals tmp = TemporaryThreadLocals.acquire()) {
-            final StringBuilder buf = tmp.stringBuilder();
-            for (int i = 0; i < escaped.length();) {
-                if (!matcher.find(i)) {
-                    buf.append(escaped, i, escaped.length());
-                    break;
-                }
-                final int pos = matcher.start();
-                buf.append(escaped, i, pos);
-                buf.append(escaped.charAt(pos) == '@' ? "%40" : "%3A");
-                i = pos + 1;
-            }
-            return buf.toString();
-        }
-    }
 }
diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/util/DomainSocketPathEscaper.java b/core/src/main/java/com/linecorp/armeria/internal/common/util/DomainSocketPathEscaper.java
@@ -0,0 +1,51 @@
+/*
+ * Copyright 2023 LINE Corporation
+ *
+ * LINE Corporation licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package com.linecorp.armeria.internal.common.util;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import com.google.common.net.UrlEscapers;
+
+public final class DomainSocketPathEscaper {
+
+    private static final Pattern AT_OR_COLON = Pattern.compile("[@:]");
+
+    public static String toAuthority(String path) {
+        path = "unix:" + requireNonNull(path, "path");
+        final String escaped = UrlEscapers.urlPathSegmentEscaper().escape(path);
+        // We need to escape `@` and `:` as well, so that the authority contains neither userinfo nor port.
+        final Matcher matcher = AT_OR_COLON.matcher(escaped);
+        try (TemporaryThreadLocals tmp = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tmp.stringBuilder();
+            for (int i = 0; i < escaped.length();) {
+                if (!matcher.find(i)) {
+                    buf.append(escaped, i, escaped.length());
+                    break;
+                }
+                final int pos = matcher.start();
+                buf.append(escaped, i, pos);
+                buf.append(escaped.charAt(pos) == '@' ? "%40" : "%3A");
+                i = pos + 1;
+            }
+            return buf.toString();
+        }
+    }
+
+    private DomainSocketPathEscaper() {}
+}
@@ -74,9 +74,6 @@ void unsupported() throws UnknownHostException {
     @Test
     void inetKeyGeneration() throws Exception {
         final String expectedKey = "foo.com:8080";
-        assertThat(SessionProtocolNegotiationCache.key(
-                "foo.com", 8080))
-                .isEqualTo(expectedKey);
         assertThat(SessionProtocolNegotiationCache.key(
                 Endpoint.of("foo.com", 8080)))
                 .isEqualTo(expectedKey);
@@ -94,7 +91,7 @@ void inetKeyGeneration() throws Exception {
 
     @Test
     void domainKeyGeneration() {
-        final String expectedKey = "unix:/var/run/foo.sock";
+        final String expectedKey = "unix%3A%2Fvar%2Frun%2Ffoo.sock";
         assertThat(SessionProtocolNegotiationCache.key(
                 Endpoint.of("unix%3A%2Fvar%2Frun%2Ffoo.sock")))
                 .isEqualTo(expectedKey);

@@ -56,14 +56,4 @@ void asEndpoint() {
             assertThat(e.toSocketAddress(0)).isEqualTo(addr);
         });
     }
-
-    @Test
-    void escape() {
-        assertThat(DomainSocketAddress.escape("@")).isEqualTo("%40");
-        assertThat(DomainSocketAddress.escape("@@")).isEqualTo("%40%40");
-        assertThat(DomainSocketAddress.escape("@foo@")).isEqualTo("%40foo%40");
-        assertThat(DomainSocketAddress.escape(":")).isEqualTo("%3A");
-        assertThat(DomainSocketAddress.escape("::")).isEqualTo("%3A%3A");
-        assertThat(DomainSocketAddress.escape(":bar:")).isEqualTo("%3Abar%3A");
-    }
 }
diff --git a/.../src/test/java/com/linecorp/armeria/internal/common/util/DomainSocketPathEscaperTest.java b/.../src/test/java/com/linecorp/armeria/internal/common/util/DomainSocketPathEscaperTest.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2023 LINE Corporation
+ *
+ * LINE Corporation licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package com.linecorp.armeria.internal.common.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.jupiter.api.Test;
+
+class DomainSocketPathEscaperTest {
+    @Test
+    void test() {
+        assertThat(DomainSocketPathEscaper.toAuthority("@")).isEqualTo("unix%3A%40");
+        assertThat(DomainSocketPathEscaper.toAuthority("@@")).isEqualTo("unix%3A%40%40");
+        assertThat(DomainSocketPathEscaper.toAuthority("@foo@")).isEqualTo("unix%3A%40foo%40");
+        assertThat(DomainSocketPathEscaper.toAuthority(":")).isEqualTo("unix%3A%3A");
+        assertThat(DomainSocketPathEscaper.toAuthority("::")).isEqualTo("unix%3A%3A%3A");
+        assertThat(DomainSocketPathEscaper.toAuthority(":bar:")).isEqualTo("unix%3A%3Abar%3A");
+    }
+}