You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@stormcrawler.apache.org by jn...@apache.org on 2026/03/30 15:12:28 UTC
(stormcrawler) 01/01: Fix race condition while having different proxies in different threads #1247
This is an automated email from the ASF dual-hosted git repository.
jnioche pushed a commit to branch 1247
in repository https://gitbox.apache.org/repos/asf/stormcrawler.git
commit 45752e303a005ff1fb212da4b2038941f9c26920
Author: Julien Nioche <ju...@digitalpebble.com>
AuthorDate: Mon Mar 30 16:10:30 2026 +0100
Fix race condition while having different proxies in different threads #1247
Signed-off-by: Julien Nioche <ju...@digitalpebble.com>
---
.../protocol/httpclient/HttpProtocol.java | 25 ++++--
.../stormcrawler/protocol/okhttp/HttpProtocol.java | 24 +++---
.../okhttp/HttpProtocolProxyConcurrencyTest.java | 96 ++++++++++++++++++++++
3 files changed, 130 insertions(+), 15 deletions(-)
diff --git a/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java b/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java
index 10c878df..a0db6ac1 100644
--- a/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java
+++ b/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java
@@ -163,44 +163,59 @@ public class HttpProtocol extends AbstractHttpProtocol
// set default request config to global config
RequestConfig reqConfig = requestConfig;
+ // default to the shared builder for non-proxy requests
+ HttpClientBuilder clientBuilder = builder;
+
// conditionally add a dynamic proxy
if (proxyManager != null) {
// retrieve proxy from proxy manager
Optional<SCProxy> proxOptional = proxyManager.getProxy(md);
if (proxOptional.isPresent()) {
SCProxy prox = proxOptional.get();
+
+ // create local copies of builders to avoid polluting shared
+ // state across concurrent requests
+ HttpClientBuilder localBuilder =
+ HttpClients.custom()
+ .setConnectionManager(CONNECTION_MANAGER)
+ .setConnectionManagerShared(true)
+ .disableRedirectHandling()
+ .disableAutomaticRetries();
+ RequestConfig.Builder localRequestConfigBuilder = RequestConfig.copy(requestConfig);
+
// 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);
+ localRequestConfigBuilder.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);
+ localBuilder.setDefaultCredentialsProvider(basicAuthCreds);
}
HttpHost proxy = new HttpHost(prox.getAddress(), Integer.parseInt(prox.getPort()));
DefaultProxyRoutePlanner routePlanner = new DefaultProxyRoutePlanner(proxy);
- builder.setRoutePlanner(routePlanner);
+ localBuilder.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();
+ reqConfig = localRequestConfigBuilder.build();
LOG.debug(
"time to build http request config with proxy: {}ms",
System.currentTimeMillis() - buildStart);
LOG.debug("fetching with " + prox.toString());
+ clientBuilder = localBuilder;
}
}
@@ -255,7 +270,7 @@ public class HttpProtocol extends AbstractHttpProtocol
// 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 = clientBuilder.build()) {
return httpclient.execute(request, responseHandler);
}
}
diff --git a/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java b/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java
index 5ddf19c0..8defb8f0 100644
--- a/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java
+++ b/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java
@@ -306,6 +306,10 @@ public class HttpProtocol extends AbstractHttpProtocol {
SCProxy prox = proxOptional.get();
// conditionally configure proxy authentication
if (StringUtils.isNotBlank(prox.getAddress())) {
+ // create a new builder from the existing client to avoid
+ // polluting shared state across concurrent requests
+ OkHttpClient.Builder localBuilder = client.newBuilder();
+
// format SCProxy into native Java proxy
Proxy proxy =
new Proxy(
@@ -314,12 +318,12 @@ public class HttpProtocol extends AbstractHttpProtocol {
prox.getAddress(), Integer.parseInt(prox.getPort())));
// set proxy in builder
- builder.proxy(proxy);
+ localBuilder.proxy(proxy);
// conditionally add proxy authentication
if (StringUtils.isNotBlank(prox.getUsername())) {
// add proxy authentication header to builder
- builder.proxyAuthenticator(
+ localBuilder.proxyAuthenticator(
(Route route, Response response) -> {
String credential =
Credentials.basic(
@@ -330,17 +334,17 @@ public class HttpProtocol extends AbstractHttpProtocol {
.build();
});
}
- }
- // save start time for debugging speed impact of client build
- long buildStart = System.currentTimeMillis();
+ // save start time for debugging speed impact of client build
+ long buildStart = System.currentTimeMillis();
- // create new local client from builder using proxy
- localClient = builder.build();
+ // create new local client from local builder using proxy
+ localClient = localBuilder.build();
- LOG.debug(
- "time to build okhttp client with proxy: {}ms",
- System.currentTimeMillis() - buildStart);
+ LOG.debug(
+ "time to build okhttp client with proxy: {}ms",
+ System.currentTimeMillis() - buildStart);
+ }
LOG.debug("fetching with proxy {} - {} ", url, prox.toString());
}
diff --git a/core/src/test/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocolProxyConcurrencyTest.java b/core/src/test/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocolProxyConcurrencyTest.java
new file mode 100644
index 00000000..d6594fec
--- /dev/null
+++ b/core/src/test/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocolProxyConcurrencyTest.java
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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
+ *
+ * http://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 org.apache.stormcrawler.protocol.okhttp;
+
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.util.Optional;
+import okhttp3.OkHttpClient;
+import org.apache.storm.Config;
+import org.apache.stormcrawler.Metadata;
+import org.apache.stormcrawler.protocol.AbstractProtocolTest;
+import org.apache.stormcrawler.proxy.ProxyManager;
+import org.apache.stormcrawler.proxy.SCProxy;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Verifies that a proxied request does not pollute the shared OkHttpClient.Builder.
+ *
+ * <p>Before the fix, {@code getProtocolOutput()} mutated the shared {@code builder} field with
+ * proxy settings. This meant that after a proxied request, any subsequent call to {@code
+ * builder.build()} (e.g., for a different proxied request) would start with the stale proxy from
+ * the previous request. Under concurrency, this causes proxy settings from one thread to leak into
+ * another's client.
+ *
+ * <p>The fix creates a per-request builder via {@code client.newBuilder()}, leaving the shared
+ * builder untouched.
+ */
+class HttpProtocolProxyConcurrencyTest extends AbstractProtocolTest {
+
+ /**
+ * After a proxied request completes, the shared builder should not retain any proxy
+ * configuration. With the old buggy code, {@code builder.proxy(proxy)} permanently set the
+ * proxy on the shared builder. With the fix, the shared builder is never touched.
+ */
+ @Test
+ void proxiedRequestShouldNotPolluteSharedBuilder() throws Exception {
+ HttpProtocol protocol = new HttpProtocol();
+ Config conf = new Config();
+ conf.put("http.agent.name", "test");
+ conf.put("http.agent.version", "1.0");
+ conf.put("http.agent.description", "test");
+ conf.put("http.agent.url", "http://test.example.com");
+ conf.put("http.agent.email", "test@example.com");
+ protocol.configure(conf);
+
+ protocol.proxyManager =
+ new ProxyManager() {
+ @Override
+ public void configure(Config conf) {}
+
+ @Override
+ public Optional<SCProxy> getProxy(Metadata metadata) {
+ return Optional.of(new SCProxy("http://127.0.0.1:19999"));
+ }
+ };
+
+ // Make a request that uses a proxy. The connection through the fake
+ // proxy will fail, but the important thing is that getProtocolOutput
+ // goes through the proxy builder path.
+ try {
+ protocol.getProtocolOutput("http://127.0.0.1:" + HTTP_PORT, Metadata.empty);
+ } catch (Exception ignored) {
+ // expected — the proxy at 127.0.0.1:19999 doesn't exist
+ }
+
+ // Now check the shared builder. With the old code, builder.proxy was
+ // set to the proxy above. With the fix, builder is untouched.
+ java.lang.reflect.Field builderField = HttpProtocol.class.getDeclaredField("builder");
+ builderField.setAccessible(true);
+ OkHttpClient.Builder sharedBuilder = (OkHttpClient.Builder) builderField.get(protocol);
+
+ // Build a client from the shared builder and verify it has no proxy.
+ // If the shared builder was polluted, this client would have a proxy.
+ OkHttpClient clientFromSharedBuilder = sharedBuilder.build();
+ assertNull(
+ clientFromSharedBuilder.proxy(),
+ "Shared builder should not retain proxy settings from a previous request. "
+ + "This indicates the bug where getProtocolOutput() mutates the shared "
+ + "builder instead of using a per-request builder.");
+ }
+}