8191169: java/net/Authenticator/B4769350.java failed intermittently
authordfuchs
Mon, 19 Aug 2019 11:14:50 +0100
changeset 57793 d372747e8f08
parent 57792 1b6806340400
child 57794 ffdb18fb88b9
8191169: java/net/Authenticator/B4769350.java failed intermittently Summary: fixed a race condition in AuthenticationInfo when serializeAuth=true Reviewed-by: chegar, michaelm
src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java
test/jdk/java/net/Authenticator/B4769350.java
test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java
test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTest.java
test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTestServer.java
--- a/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java	Mon Aug 19 06:13:52 2019 +0200
+++ b/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java	Mon Aug 19 11:14:50 2019 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,6 +31,7 @@
 import java.net.URL;
 import java.util.HashMap;
 import java.util.Objects;
+import java.util.function.Function;
 
 import sun.net.www.HeaderParser;
 
@@ -125,25 +126,42 @@
      */
     private static HashMap<String,Thread> requests = new HashMap<>();
 
-    /* check if a request for this destination is in progress
-     * return false immediately if not. Otherwise block until
-     * request is finished and return true
+    /*
+     * check if AuthenticationInfo is available in the cache.
+     * If not, check if a request for this destination is in progress
+     * and if so block until the other request is finished authenticating
+     * and returns the cached authentication value.
+     * Otherwise, returns the cached authentication value, which may be null.
      */
-    private static boolean requestIsInProgress (String key) {
-        if (!serializeAuth) {
-            /* behavior is disabled. Revert to concurrent requests */
-            return false;
+    private static AuthenticationInfo requestAuthentication(String key, Function<String, AuthenticationInfo> cache) {
+        AuthenticationInfo cached = cache.apply(key);
+        if (cached != null || !serializeAuth) {
+            // either we already have a value in the cache, and we can
+            // use that immediately, or the serializeAuth behavior is disabled,
+            // and we can revert to concurrent requests
+            return cached;
         }
         synchronized (requests) {
+            // check again after synchronizing, and if available
+            // just return the cached value.
+            cached = cache.apply(key);
+            if (cached != null) return cached;
+
+            // Otherwise, if no request is in progress, record this
+            // thread as performing authentication and returns null.
             Thread t, c;
             c = Thread.currentThread();
             if ((t = requests.get(key)) == null) {
                 requests.put (key, c);
-                return false;
+                assert cached == null;
+                return cached;
             }
             if (t == c) {
-                return false;
+                assert cached == null;
+                return cached;
             }
+            // Otherwise, an other thread is currently performing authentication:
+            // wait until it finishes.
             while (requests.containsKey(key)) {
                 try {
                     requests.wait ();
@@ -151,7 +169,7 @@
             }
         }
         /* entry may be in cache now. */
-        return true;
+        return cache.apply(key);
     }
 
     /* signal completion of an authentication (whether it succeeded or not)
@@ -318,13 +336,13 @@
         return key;
     }
 
+    private static AuthenticationInfo getCachedServerAuth(String key) {
+        return getAuth(key, null);
+    }
+
     static AuthenticationInfo getServerAuth(String key) {
-        AuthenticationInfo cached = getAuth(key, null);
-        if ((cached == null) && requestIsInProgress (key)) {
-            /* check the cache again, it might contain an entry */
-            cached = getAuth(key, null);
-        }
-        return cached;
+        if (!serializeAuth) return getCachedServerAuth(key);
+        return requestAuthentication(key, AuthenticationInfo::getCachedServerAuth);
     }
 
 
@@ -367,13 +385,13 @@
         return key;
     }
 
+    private static AuthenticationInfo getCachedProxyAuth(String key) {
+        return (AuthenticationInfo) cache.get(key, null);
+    }
+
     static AuthenticationInfo getProxyAuth(String key) {
-        AuthenticationInfo cached = (AuthenticationInfo) cache.get(key, null);
-        if ((cached == null) && requestIsInProgress (key)) {
-            /* check the cache again, it might contain an entry */
-            cached = (AuthenticationInfo) cache.get(key, null);
-        }
-        return cached;
+        if (!serializeAuth) return getCachedProxyAuth(key);
+        return requestAuthentication(key, AuthenticationInfo::getCachedProxyAuth);
     }
 
 
--- a/test/jdk/java/net/Authenticator/B4769350.java	Mon Aug 19 06:13:52 2019 +0200
+++ b/test/jdk/java/net/Authenticator/B4769350.java	Mon Aug 19 11:14:50 2019 +0100
@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 4769350 8017779
+ * @bug 4769350 8017779 8191169
  * @modules jdk.httpserver
  * @run main/othervm B4769350 server
  * @run main/othervm B4769350 proxy
--- a/test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java	Mon Aug 19 06:13:52 2019 +0200
+++ b/test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java	Mon Aug 19 11:14:50 2019 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -115,8 +115,8 @@
                    HttpAuthType mode)
             throws IOException
     {
-        HttpTestAuthenticator authOne = new HttpTestAuthenticator("dublin", "foox");
-        HttpTestAuthenticator authTwo = new HttpTestAuthenticator("dublin", "foox");
+        HttpTestAuthenticator authOne = new HttpTestAuthenticator("authOne", "dublin", "foox");
+        HttpTestAuthenticator authTwo = new HttpTestAuthenticator("authTwo", "dublin", "foox");
         int expectedIncrement = scheme == HttpSchemeType.NONE
                                 ? 0 : EXPECTED_AUTH_CALLS_PER_TEST;
         int count;
--- a/test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTest.java	Mon Aug 19 06:13:52 2019 +0200
+++ b/test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTest.java	Mon Aug 19 11:14:50 2019 +0100
@@ -89,8 +89,10 @@
         // count will be incremented every time getPasswordAuthentication()
         // is called from the client side.
         final AtomicInteger count = new AtomicInteger();
+        private final String name;
 
-        public HttpTestAuthenticator(String realm, String username) {
+        public HttpTestAuthenticator(String name, String realm, String username) {
+            this.name = name;
             this.realm = realm;
             this.username = username;
         }
@@ -98,7 +100,7 @@
         @Override
         protected PasswordAuthentication getPasswordAuthentication() {
             if (skipCount.get() == null || skipCount.get().booleanValue() == false) {
-                System.out.println("Authenticator called: " + count.incrementAndGet());
+                System.out.println("Authenticator " + name + " called: " + count.incrementAndGet());
             }
             return new PasswordAuthentication(getUserName(),
                     new char[] {'b','a','r'});
@@ -118,6 +120,11 @@
             throw new SecurityException("User unknown: " + user);
         }
 
+        @Override
+        public String toString() {
+            return super.toString() + "[name=\"" + name + "\"]";
+        }
+
         public final String getUserName() {
             return username;
         }
@@ -128,7 +135,7 @@
     }
     public static final HttpTestAuthenticator AUTHENTICATOR;
     static {
-        AUTHENTICATOR = new HttpTestAuthenticator("dublin", "foox");
+        AUTHENTICATOR = new HttpTestAuthenticator("AUTHENTICATOR","dublin", "foox");
         Authenticator.setDefault(AUTHENTICATOR);
     }
 
--- a/test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTestServer.java	Mon Aug 19 06:13:52 2019 +0200
+++ b/test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTestServer.java	Mon Aug 19 11:14:50 2019 +0100
@@ -165,7 +165,7 @@
                 for (int i = 1; i <= max; i++) {
                     B bindable = createBindable();
                     SocketAddress address = getAddress(bindable);
-                    String key = address.toString();
+                    String key = toString(address);
                     if (addresses.addIfAbsent(key)) {
                        System.out.println("Socket bound to: " + key
                                           + " after " + i + " attempt(s)");
@@ -188,6 +188,16 @@
                                   + "addresses used before: " + addresses);
         }
 
+        private static String toString(SocketAddress address) {
+            // We don't rely on address.toString(): sometimes it can be
+            // "/127.0.0.1:port", sometimes it can be "localhost/127.0.0.1:port"
+            // Instead we compose our own string representation:
+            InetSocketAddress candidate = (InetSocketAddress) address;
+            String hostAddr = candidate.getAddress().getHostAddress();
+            if (hostAddr.contains(":")) hostAddr = "[" + hostAddr + "]";
+            return hostAddr + ":" + candidate.getPort();
+        }
+
         protected abstract B createBindable() throws IOException;
 
         protected abstract SocketAddress getAddress(B bindable);