Skip to content

Commit

Permalink
Fix race condition in HttpProtocol while having multiple proxy settings
Browse files Browse the repository at this point in the history 
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 apache#1247
  • Loading branch information
@chhsiao90
chhsiao90 committed Jul 8, 2024
1 parent ef0899e commit d5c445f
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 143 deletions.
109 changes: 45 additions & 64 deletions core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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);

Expand All @@ -132,71 +127,29 @@ 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 =
RequestConfig.custom()
.setSocketTimeout(timeout)
.setConnectTimeout(timeout)
.setConnectionRequestTimeout(timeout)
.setCookieSpec(CookieSpecs.STANDARD);

requestConfig = requestConfigBuilder.build();
.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);
Expand Down Expand Up @@ -246,11 +199,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);
}
}
Expand Down Expand Up @@ -372,6 +325,34 @@ 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);
}
Expand Down
Loading

0 comments on commit d5c445f

Please sign in to comment.