Fix race condition in HttpProtocol while having multiple proxy settings

In HttpProtocol implementation, the client builder was singleton and may
be accessed and modified by different threads at same time. The result
is that a wrong proxy will be used or a wrong proxy auth will be
configured.

To fix it, create a local builder insteand of having a field-level
builder.

Fixes #1247

Author: chhsiao90

core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java

@@ -22,8 +22,7 @@
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Base64;
-import java.util.Collection;
-import java.util.LinkedList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import org.apache.commons.lang.StringUtils;
@@ -78,10 +77,11 @@ public class HttpProtocol extends AbstractHttpProtocol
 
     private int globalMaxContent;
 
-    private HttpClientBuilder builder;
-
     private RequestConfig requestConfig;
-    private RequestConfig.Builder requestConfigBuilder;
+
+    private String userAgent;
+
+    private final List<BasicHeader> defaultHeaders = new ArrayList<>();
 
     @Override
     public void configure(final Config conf) {
@@ -100,19 +100,14 @@ public void configure(final Config conf) {
 
         globalMaxContent = ConfUtils.getInt(conf, "http.content.limit", -1);
 
-        String userAgent = getAgentString(conf);
-
-        Collection<BasicHeader> defaultHeaders = new LinkedList<>();
+        userAgent = getAgentString(conf);
 
         String accept = ConfUtils.getString(conf, "http.accept");
         if (StringUtils.isNotBlank(accept)) {
             defaultHeaders.add(new BasicHeader("Accept", accept));
         }
 
-        customHeaders.forEach(
-                h -> {
-                    defaultHeaders.add(new BasicHeader(h.getKey(), h.getValue()));
-                });
+        customHeaders.forEach(h -> defaultHeaders.add(new BasicHeader(h.getKey(), h.getValue())));
 
         String basicAuthUser = ConfUtils.getString(conf, "http.basicauth.user", null);
 
@@ -132,71 +127,28 @@ public void configure(final Config conf) {
             defaultHeaders.add(new BasicHeader("Accept-Language", acceptLanguage));
         }
 
-        builder =
-                HttpClients.custom()
-                        .setUserAgent(userAgent)
-                        .setDefaultHeaders(defaultHeaders)
-                        .setConnectionManager(CONNECTION_MANAGER)
-                        .setConnectionManagerShared(true)
-                        .disableRedirectHandling()
-                        .disableAutomaticRetries();
-
         int timeout = ConfUtils.getInt(conf, "http.timeout", 10000);
 
-        requestConfigBuilder =
-                RequestConfig.custom()
-                        .setSocketTimeout(timeout)
-                        .setConnectTimeout(timeout)
-                        .setConnectionRequestTimeout(timeout)
-                        .setCookieSpec(CookieSpecs.STANDARD);
-
-        requestConfig = requestConfigBuilder.build();
+        requestConfig = RequestConfig.custom()
+                .setSocketTimeout(timeout)
+                .setConnectTimeout(timeout)
+                .setConnectionRequestTimeout(timeout)
+                .setCookieSpec(CookieSpecs.STANDARD)
+                // Can make configurable and add more in future
+                .setProxyPreferredAuthSchemes(Collections.singletonList(AuthSchemes.BASIC))
+                .build();
     }
 
     @Override
     public ProtocolResponse getProtocolOutput(String url, Metadata md) throws Exception {
 
         LOG.debug("HTTP connection manager stats {}", CONNECTION_MANAGER.getTotalStats());
 
-        // set default request config to global config
-        RequestConfig reqConfig = requestConfig;
-
+        SCProxy proxy = null;
         // conditionally add a dynamic proxy
         if (proxyManager != null) {
-            // retrieve proxy from proxy manager
-            SCProxy prox = proxyManager.getProxy(md);
-
-            // conditionally configure proxy authentication
-            if (StringUtils.isNotBlank(prox.getUsername())) {
-                List<String> authSchemes = new ArrayList<>();
-
-                // Can make configurable and add more in future
-                authSchemes.add(AuthSchemes.BASIC);
-                requestConfigBuilder.setProxyPreferredAuthSchemes(authSchemes);
-
-                BasicCredentialsProvider basicAuthCreds = new BasicCredentialsProvider();
-                basicAuthCreds.setCredentials(
-                        new AuthScope(prox.getAddress(), Integer.parseInt(prox.getPort())),
-                        new UsernamePasswordCredentials(prox.getUsername(), prox.getPassword()));
-                builder.setDefaultCredentialsProvider(basicAuthCreds);
-            }
-
-            HttpHost proxy = new HttpHost(prox.getAddress(), Integer.parseInt(prox.getPort()));
-            DefaultProxyRoutePlanner routePlanner = new DefaultProxyRoutePlanner(proxy);
-            builder.setRoutePlanner(routePlanner);
-
-            // save start time for debugging speed impact of request config
-            // build
-            long buildStart = System.currentTimeMillis();
-
-            // set request config to new configuration with dynamic proxy
-            reqConfig = requestConfigBuilder.build();
-
-            LOG.debug(
-                    "time to build http request config with proxy: {}ms",
-                    System.currentTimeMillis() - buildStart);
-
-            LOG.debug("fetching with " + prox.toString());
+            proxy = proxyManager.getProxy(md);
+            LOG.debug("fetching with {}", proxy);
         }
 
         HttpRequestBase request = new HttpGet(url);
@@ -246,11 +198,11 @@ public ProtocolResponse getProtocolOutput(String url, Metadata md) throws Except
             }
         }
 
-        request.setConfig(reqConfig);
+        request.setConfig(requestConfig);
 
         // no need to release the connection explicitly as this is handled
         // automatically. The client itself must be closed though.
-        try (CloseableHttpClient httpclient = builder.build()) {
+        try (CloseableHttpClient httpclient = createClient(proxy)) {
             return httpclient.execute(request, responseHandler);
         }
     }
@@ -372,6 +324,32 @@ private static byte[] toByteArray(
         return buffer.toByteArray();
     }
 
+    private CloseableHttpClient createClient(final SCProxy proxy) {
+        final HttpClientBuilder builder = HttpClients.custom()
+                .setUserAgent(userAgent)
+                .setDefaultHeaders(defaultHeaders)
+                .setConnectionManager(CONNECTION_MANAGER)
+                .setConnectionManagerShared(true)
+                .disableRedirectHandling()
+                .disableAutomaticRetries();
+
+        if (proxy != null) {
+            final DefaultProxyRoutePlanner routePlanner = new DefaultProxyRoutePlanner(new HttpHost(
+                    proxy.getAddress(), Integer.parseInt(proxy.getPort())));
+            builder.setRoutePlanner(routePlanner);
+            if (StringUtils.isNotBlank(proxy.getUsername())) {
+                // conditionally configure proxy authentication
+                final BasicCredentialsProvider credentialsProvider = new BasicCredentialsProvider();
+                credentialsProvider.setCredentials(
+                        new AuthScope(proxy.getAddress(), Integer.parseInt(proxy.getPort())),
+                        new UsernamePasswordCredentials(proxy.getUsername(), proxy.getPassword()));
+                builder.setDefaultCredentialsProvider(credentialsProvider);
+            }
+        }
+
+        return builder.build();
+    }
+
     public static void main(String[] args) throws Exception {
         Protocol.main(new HttpProtocol(), args);
     }