Merge branch 'main' into issue-4382-new

.github/workflows/pr-stale.yml

@@ -0,0 +1,20 @@
+name: 'Mark inactive PRs as stale'
+on:
+  schedule:
+    # Update at 10:00 AM UTC+9
+    - cron: '0 1 * * *'
+
+jobs:
+  stale:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/stale@v9
+        with:
+          # Mark PRs as stale if they are open and inactive for 30 days.
+          days-before-pr-stale: 30
+          # Do not leave a message when marking PRs as stale.
+          stale-pr-message: ''
+          # Do not mark issues as stale.
+          days-before-issue-stale: -1
+          # Do not close PRs automatically.
+          days-before-close: -1

core/build.gradle

@@ -61,17 +61,23 @@ tasks.withType(Jar) {
 tasks.trimShadedJar.doLast {
     // outjars is a file, so only one jar generated for sure
     def trimmed = tasks.trimShadedJar.outJarFiles[0].toPath()
-    ant.jar(destfile: trimmed.toString(), update: true) {
+
+    ant.jar(destfile: trimmed.toString(), update: true, duplicate: 'fail') {
         zipfileset(src: tasks.shadedJar.archivePath) {
             include(name: 'META-INF/versions/**')
         }
+
+        // Do not let Ant put the properties that harm the build reproducibility, such as JDK version.
+        delegate.manifest {
+            attribute(name: 'Created-By', value: "Gradle ${gradle.gradleVersion}")
+        }
     }
 }
 
 tasks.shadedTest.exclude 'META-INF/versions/**'
 
 dependencies {
-    mrJarVersions.each { version->
+    mrJarVersions.each { version ->
         // Common to reference classes in main sourceset from Java 9 one (e.g., to return a common interface)
         "java${version}Implementation" files(sourceSets.main.output.classesDirs) { builtBy compileJava }
 

core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java

@@ -22,6 +22,8 @@
 import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.findAuthority;
 
 import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.function.BiPredicate;
@@ -30,8 +32,6 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.LinkedListMultimap;
-import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
 
 import com.linecorp.armeria.client.redirect.CyclicRedirectsException;
@@ -263,18 +263,12 @@ private void execute0(ClientRequestContext ctx, RedirectContext redirectCtx,
                     newReqDuplicator(reqDuplicator, responseHeaders, requestHeaders,
                                      nextReqTarget.toString(), nextAuthority);
 
-            if (isCyclicRedirects(redirectCtx, nextReqTarget.toString(), newReqDuplicator.headers().method())) {
-                handleException(ctx, derivedCtx, reqDuplicator, responseFuture, response,
-                                CyclicRedirectsException.of(redirectCtx.originalUri(),
-                                                            redirectCtx.redirectUris().values()));
-                return;
-            }
-
-            final Multimap<HttpMethod, String> redirectUris = redirectCtx.redirectUris();
-            if (redirectUris.size() > maxRedirects) {
-                handleException(ctx, derivedCtx, reqDuplicator, responseFuture,
-                                response, TooManyRedirectsException.of(maxRedirects, redirectCtx.originalUri(),
-                                                                       redirectUris.values()));
+            try {
+                redirectCtx.validateRedirects(nextReqTarget,
+                                              newReqDuplicator.headers().method(),
+                                              maxRedirects);
+            } catch (Throwable t) {
+                handleException(ctx, derivedCtx, reqDuplicator, responseFuture, response, t);
                 return;
             }
 
@@ -469,17 +463,6 @@ private static void abortResponse(HttpResponse originalRes, ClientRequestContext
         }
     }
 
-    private static boolean isCyclicRedirects(RedirectContext redirectCtx,
-                                             String redirectUri, HttpMethod method) {
-        final boolean added = redirectCtx.addRedirectUri(method, redirectUri);
-        if (!added) {
-            return true;
-        }
-
-        return redirectCtx.originalUri().equals(redirectUri) &&
-               redirectCtx.request().method() == method;
-    }
-
     private static String buildUri(ClientRequestContext ctx, RequestHeaders headers) {
         final String originalUri;
         try (TemporaryThreadLocals threadLocals = TemporaryThreadLocals.acquire()) {
@@ -549,9 +532,9 @@ static class RedirectContext {
         private final CompletableFuture<Void> responseWhenComplete;
         private final CompletableFuture<HttpResponse> responseFuture;
         @Nullable
-        private Multimap<HttpMethod, String> redirectUris;
-        @Nullable
         private String originalUri;
+        @Nullable
+        private Set<RedirectSignature> redirectSignatures;
 
         RedirectContext(ClientRequestContext ctx, HttpRequest request,
                         HttpResponse response, CompletableFuture<HttpResponse> responseFuture) {
@@ -573,24 +556,85 @@ CompletableFuture<HttpResponse> responseFuture() {
             return responseFuture;
         }
 
+        void validateRedirects(RequestTarget nextReqTarget, HttpMethod nextMethod, int maxRedirects) {
+            if (redirectSignatures == null) {
+                redirectSignatures = new LinkedHashSet<>();
+
+                final String originalProtocol = ctx.sessionProtocol().isTls() ? "https" : "http";
+                final RedirectSignature originalSignature = new RedirectSignature(originalProtocol,
+                                                                                  ctx.authority(),
+                                                                                  request.headers().path(),
+                                                                                  request.method());
+                redirectSignatures.add(originalSignature);
+            }
+
+            final RedirectSignature signature = new RedirectSignature(nextReqTarget.scheme(),
+                                                                      nextReqTarget.authority(),
+                                                                      nextReqTarget.pathAndQuery(),
+                                                                      nextMethod);
+            if (!redirectSignatures.add(signature)) {
+                throw CyclicRedirectsException.of(originalUri(), redirectUris());
+            }
+
+            // Minus 1 because the original signature is also included.
+            if (redirectSignatures.size() - 1 > maxRedirects) {
+                throw TooManyRedirectsException.of(maxRedirects, originalUri(), redirectUris());
+            }
+        }
+
         String originalUri() {
             if (originalUri == null) {
                 originalUri = buildUri(ctx, request.headers());
             }
             return originalUri;
         }
 
-        boolean addRedirectUri(HttpMethod method, String redirectUri) {
-            if (redirectUris == null) {
-                redirectUris = LinkedListMultimap.create();
+        Set<String> redirectUris() {
+            // Always called after addRedirectSignature is called.
+            assert redirectSignatures != null;
+            return redirectSignatures.stream()
+                                     .map(RedirectSignature::uri)
+                                     .collect(ImmutableSet.toImmutableSet());
+        }
+    }
+
+    @VisibleForTesting
+    static class RedirectSignature {
+        private final String protocol;
+        private final String authority;
+        private final String pathAndQuery;
+        private final HttpMethod method;
+
+        RedirectSignature(String protocol, String authority, String pathAndQuery, HttpMethod method) {
+            this.protocol = protocol;
+            this.authority = authority;
+            this.pathAndQuery = pathAndQuery;
+            this.method = method;
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(protocol, authority, pathAndQuery, method);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (obj == this) {
+                return true;
+            }
+            if (!(obj instanceof RedirectSignature)) {
+                return false;
             }
-            return redirectUris.put(method, redirectUri);
+
+            final RedirectSignature that = (RedirectSignature) obj;
+            return pathAndQuery.equals(that.pathAndQuery) &&
+                   authority.equals(that.authority) &&
+                   protocol.equals(that.protocol) &&
+                   method == that.method;
         }
 
-        Multimap<HttpMethod, String> redirectUris() {
-            // Always called after addRedirectUri is called.
-            assert redirectUris != null;
-            return redirectUris;
+        String uri() {
+            return protocol + "://" + authority + pathAndQuery;
         }
     }
 }