Make TemporaryThreadLocals AutoClosable to safely use shared reso…

…urces (#3505) Motivation: - When a developer want to use shared resources in `TemporaryThreadLocals`, it is hard to ensure whether there is nested use to cause a corruption. Modifications: - Make `TemporaryThreadLocals` `AutoClosable` with lock. - Rename `TemporaryThreadLocals.get()` to `TemporaryThreadLocals.acquire()` with `@MustBeClosed` annotation. - Apply try-with-resources statement to `TemporaryThreadLocals.acquire()` usages. - nit. Fix the order about methods in `TemporaryThreadLocals`. Result: - If a developer try to use before releasing the resource, `IllegalStateException` occurs. - Not only a developer, but also reviewers feel easy to use without concerns.
line · Jun 14, 2021 · d4880fe · d4880fe
1 parent 42bd0d7
commit d4880fe
Show file tree

Hide file tree

Showing 29 changed files with 682 additions and 559 deletions.
diff --git a/benchmarks/src/jmh/java/com/linecorp/armeria/common/QueryStringEncoderBenchmark.java b/benchmarks/src/jmh/java/com/linecorp/armeria/common/QueryStringEncoderBenchmark.java
@@ -120,14 +120,16 @@ public void guavaLong(Blackhole bh) {
     }
 
     private static String guavaEncode(QueryParamGetters params) {
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        for (Entry<String, String> e : params) {
-            buf.append(guavaEscaper.escape(e.getKey()))
-               .append('=')
-               .append(guavaEscaper.escape(e.getValue()))
-               .append('&');
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            for (Entry<String, String> e : params) {
+                buf.append(guavaEscaper.escape(e.getKey()))
+                   .append('=')
+                   .append(guavaEscaper.escape(e.getValue()))
+                   .append('&');
+            }
+            return buf.substring(0, buf.length() - 1);
         }
-        return buf.substring(0, buf.length() - 1);
     }
 
     @Benchmark
@@ -179,13 +181,15 @@ public void jdkLong(Blackhole bh) throws UnsupportedEncodingException {
     }
 
     private static String jdkEncode(QueryParamGetters params) throws UnsupportedEncodingException {
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        for (Entry<String, String> e : params) {
-            buf.append(URLEncoder.encode(e.getKey(), "UTF-8"))
-               .append('=')
-               .append(URLEncoder.encode(e.getValue(), "UTF-8"))
-               .append('&');
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            for (Entry<String, String> e : params) {
+                buf.append(URLEncoder.encode(e.getKey(), "UTF-8"))
+                   .append('=')
+                   .append(URLEncoder.encode(e.getValue(), "UTF-8"))
+                   .append('&');
+            }
+            return buf.substring(0, buf.length() - 1);
         }
-        return buf.substring(0, buf.length() - 1);
     }
 }
diff --git a/core/src/main/java/com/linecorp/armeria/client/DefaultClientBuilderParams.java b/core/src/main/java/com/linecorp/armeria/client/DefaultClientBuilderParams.java
@@ -54,15 +54,17 @@ final class DefaultClientBuilderParams implements ClientBuilderParams {
         scheme = factory.validateScheme(Scheme.parse(uri.getScheme()));
         endpointGroup = Endpoint.parse(uri.getRawAuthority());
 
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        buf.append(nullOrEmptyToSlash(uri.getRawPath()));
-        if (uri.getRawQuery() != null) {
-            buf.append('?').append(uri.getRawQuery());
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            buf.append(nullOrEmptyToSlash(uri.getRawPath()));
+            if (uri.getRawQuery() != null) {
+                buf.append('?').append(uri.getRawQuery());
+            }
+            if (uri.getRawFragment() != null) {
+                buf.append('#').append(uri.getRawFragment());
+            }
+            absolutePathRef = buf.toString();
         }
-        if (uri.getRawFragment() != null) {
-            buf.append('#').append(uri.getRawFragment());
-        }
-        absolutePathRef = buf.toString();
     }
 
     DefaultClientBuilderParams(Scheme scheme, EndpointGroup endpointGroup,

diff --git a/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java
@@ -694,25 +694,27 @@ private String toStringSlow(@Nullable Channel ch, @Nullable RequestLogAccess par
         final String method = method().name();
 
         // Build the string representation.
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        buf.append("[creqId=").append(creqId);
-        if (parent != null) {
-            buf.append(", preqId=").append(preqId);
-        }
-        if (sreqId != null) {
-            buf.append(", sreqId=").append(sreqId);
-        }
-        if (ch != null) {
-            buf.append(", chanId=").append(chanId)
-               .append(", laddr=");
-            TextFormatter.appendSocketAddress(buf, ch.localAddress());
-            buf.append(", raddr=");
-            TextFormatter.appendSocketAddress(buf, ch.remoteAddress());
-        }
-        buf.append("][")
-           .append(proto).append("://").append(authority).append(path).append('#').append(method)
-           .append(']');
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            buf.append("[creqId=").append(creqId);
+            if (parent != null) {
+                buf.append(", preqId=").append(preqId);
+            }
+            if (sreqId != null) {
+                buf.append(", sreqId=").append(sreqId);
+            }
+            if (ch != null) {
+                buf.append(", chanId=").append(chanId)
+                   .append(", laddr=");
+                TextFormatter.appendSocketAddress(buf, ch.localAddress());
+                buf.append(", raddr=");
+                TextFormatter.appendSocketAddress(buf, ch.remoteAddress());
+            }
+            buf.append("][")
+               .append(proto).append("://").append(authority).append(path).append('#').append(method)
+               .append(']');
 
-        return buf.toString();
+            return buf.toString();
+        }
     }
 }
diff --git a/core/src/main/java/com/linecorp/armeria/client/Endpoint.java b/core/src/main/java/com/linecorp/armeria/client/Endpoint.java
@@ -189,13 +189,15 @@ private static String generateAuthority(String host, int port, HostType hostType
 
     private static String generateToString(String authority, @Nullable String ipAddr,
                                            int weight, HostType hostType) {
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        buf.append("Endpoint{").append(authority);
-        if (hostType == HostType.HOSTNAME_AND_IPv4 ||
-            hostType == HostType.HOSTNAME_AND_IPv6) {
-            buf.append(", ipAddr=").append(ipAddr);
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            buf.append("Endpoint{").append(authority);
+            if (hostType == HostType.HOSTNAME_AND_IPv4 ||
+                hostType == HostType.HOSTNAME_AND_IPv6) {
+                buf.append(", ipAddr=").append(ipAddr);
+            }
+            return buf.append(", weight=").append(weight).append('}').toString();
         }
-        return buf.append(", weight=").append(weight).append('}').toString();
     }
 
     @Override

diff --git a/core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java b/core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java
@@ -254,16 +254,18 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
     }
 
     private void failWithUnexpectedMessageType(ChannelHandlerContext ctx, Object msg, Class<?> expected) {
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        buf.append("unexpected message type: " + msg.getClass().getName() +
-                   " (expected: " + expected.getName() + ", channel: " + ctx.channel() +
-                   ", resId: " + resId);
-        if (lastPingReqId == -1) {
-            buf.append(')');
-        } else {
-            buf.append(", lastPingReqId: " + lastPingReqId + ')');
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            buf.append("unexpected message type: " + msg.getClass().getName() +
+                       " (expected: " + expected.getName() + ", channel: " + ctx.channel() +
+                       ", resId: " + resId);
+            if (lastPingReqId == -1) {
+                buf.append(')');
+            } else {
+                buf.append(", lastPingReqId: " + lastPingReqId + ')');
+            }
+            fail(ctx, new ProtocolViolationException(buf.toString()));
         }
-        fail(ctx, new ProtocolViolationException(buf.toString()));
     }
 
     private void fail(ChannelHandlerContext ctx, Throwable cause) {

diff --git a/core/src/main/java/com/linecorp/armeria/common/AbstractHttpRequestBuilder.java b/core/src/main/java/com/linecorp/armeria/common/AbstractHttpRequestBuilder.java
@@ -123,6 +123,7 @@ public AbstractHttpRequestBuilder trace(String path) {
 
     /**
      * Sets the method for this request.
+     *
      * @see HttpMethod
      */
     public AbstractHttpRequestBuilder method(HttpMethod method) {
@@ -226,6 +227,7 @@ public AbstractHttpRequestBuilder header(CharSequence name, Object value) {
      *            .headers(HttpHeaders.of("authorization", "foo", "bar", "baz"))
      *            .build();
      * }</pre>
+     *
      * @see HttpHeaders
      */
     public AbstractHttpRequestBuilder headers(
@@ -331,6 +333,7 @@ public AbstractHttpRequestBuilder queryParam(String name, Object value) {
      *            .queryParams(QueryParams.of("from", "foo", "limit", 10))
      *            .build(); // GET `/endpoint?from=foo&limit=10`
      * }</pre>
+     *
      * @see QueryParams
      */
     public AbstractHttpRequestBuilder queryParams(
@@ -351,6 +354,7 @@ public AbstractHttpRequestBuilder queryParams(
      *            .cookie(Cookie.of("cookie", "foo"))
      *            .build();
      * }</pre>
+     *
      * @see Cookie
      */
     public AbstractHttpRequestBuilder cookie(Cookie cookie) {
@@ -371,6 +375,7 @@ public AbstractHttpRequestBuilder cookie(Cookie cookie) {
      *                                Cookie.of("cookie2", "bar")))
      *            .build();
      * }</pre>
+     *
      * @see Cookies
      */
     public AbstractHttpRequestBuilder cookies(Iterable<? extends Cookie> cookies) {
@@ -476,81 +481,83 @@ private String buildPath() {
 
             if (hasPathParams) {
                 // Replace path parameters.
-                final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-                buf.append(path, 0, i);
-
-                loop:
-                while (i < pathLen) {
-                    final char ch = path.charAt(i);
-                    switch (ch) {
-                        case '{': {
-                            int j = i + 1;
-                            // Find the matching '}'
-                            while (j < pathLen && path.charAt(j) != '}') {
-                                j++;
-                            }
+                try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+                    final StringBuilder buf = tempThreadLocals.stringBuilder();
+                    buf.append(path, 0, i);
+
+                    loop:
+                    while (i < pathLen) {
+                        final char ch = path.charAt(i);
+                        switch (ch) {
+                            case '{': {
+                                int j = i + 1;
+                                // Find the matching '}'
+                                while (j < pathLen && path.charAt(j) != '}') {
+                                    j++;
+                                }
 
-                            if (j >= pathLen) {
-                                // Found no matching '}'
-                                buf.append(path, i, pathLen);
-                                break loop;
-                            }
+                                if (j >= pathLen) {
+                                    // Found no matching '}'
+                                    buf.append(path, i, pathLen);
+                                    break loop;
+                                }
 
-                            if (j > i + 1) {
-                                final String name = path.substring(i + 1, j);
-                                checkState(pathParams != null && pathParams.containsKey(name),
-                                           "param '%s' does not have a value.", name);
-                                buf.append(pathParams.get(name));
-                                j++; // Skip '}'
-                            } else {
-                                // Found '{}'
-                                j++; // Skip '}'
-                                buf.append('{').append('}');
+                                if (j > i + 1) {
+                                    final String name = path.substring(i + 1, j);
+                                    checkState(pathParams != null && pathParams.containsKey(name),
+                                               "param '%s' does not have a value.", name);
+                                    buf.append(pathParams.get(name));
+                                    j++; // Skip '}'
+                                } else {
+                                    // Found '{}'
+                                    j++; // Skip '}'
+                                    buf.append('{').append('}');
+                                }
+                                i = j;
+                                break;
                             }
-                            i = j;
-                            break;
-                        }
-                        case ':': {
-                            int j = i + 1;
-                            while (j < pathLen && path.charAt(j) != '/' && path.charAt(j) != '?') {
-                                j++;
+                            case ':': {
+                                int j = i + 1;
+                                while (j < pathLen && path.charAt(j) != '/' && path.charAt(j) != '?') {
+                                    j++;
+                                }
+                                if (j > i + 1) {
+                                    final String name = path.substring(i + 1, j);
+                                    checkState(pathParams != null && pathParams.containsKey(name),
+                                               "param '%s' does not have a value.", name);
+                                    buf.append(pathParams.get(name));
+                                } else {
+                                    // Found ':' without name.
+                                    buf.append(':');
+                                }
+                                i = j;
+                                break;
                             }
-                            if (j > i + 1) {
-                                final String name = path.substring(i + 1, j);
-                                checkState(pathParams != null && pathParams.containsKey(name),
-                                           "param '%s' does not have a value.", name);
-                                buf.append(pathParams.get(name));
-                            } else {
-                                // Found ':' without name.
-                                buf.append(':');
+                            case '?': {
+                                hasQueryInPath = true;
+                                buf.append(path, i, pathLen);
+                                break loop;
                             }
-                            i = j;
-                            break;
+                            default:
+                                buf.append(ch);
+                                i++;
                         }
-                        case '?': {
-                            hasQueryInPath = true;
-                            buf.append(path, i, pathLen);
-                            break loop;
-                        }
-                        default:
-                            buf.append(ch);
-                            i++;
                     }
-                }
 
-                if (hasQueryInPath) {
-                    if (queryParams != null) {
-                        buf.append('&');
-                        queryParams.appendQueryString(buf);
-                    }
-                } else {
-                    if (queryParams != null) {
-                        buf.append('?');
-                        queryParams.appendQueryString(buf);
+                    if (hasQueryInPath) {
+                        if (queryParams != null) {
+                            buf.append('&');
+                            queryParams.appendQueryString(buf);
+                        }
+                    } else {
+                        if (queryParams != null) {
+                            buf.append('?');
+                            queryParams.appendQueryString(buf);
+                        }
                     }
-                }
 
-                return buf.toString();
+                    return buf.toString();
+                }
             } else {
                 // path doesn't contain a path parameter.
                 if (queryParams != null) {
@@ -570,10 +577,11 @@ private String buildPath() {
 
     private static String buildPathWithoutPathParams(
             String path, QueryParamsBuilder queryParams, boolean hasQueryInPath) {
-
-        final StringBuilder buf = TemporaryThreadLocals.get().stringBuilder();
-        buf.append(path).append(hasQueryInPath ? '&' : '?');
-        queryParams.appendQueryString(buf);
-        return buf.toString();
+        try (TemporaryThreadLocals tempThreadLocals = TemporaryThreadLocals.acquire()) {
+            final StringBuilder buf = tempThreadLocals.stringBuilder();
+            buf.append(path).append(hasQueryInPath ? '&' : '?');
+            queryParams.appendQueryString(buf);
+            return buf.toString();
+        }
     }
 }