java http client is unfortunately sometimes not honoring timeouts (https://bugs.openjdk.org/browse/JDK-8258397), use httpclient again

This commit is contained in:
Athou
2023-12-25 13:47:19 +01:00
parent 07dd10848f
commit 11aff68052
2 changed files with 88 additions and 84 deletions

View File

@@ -1,25 +1,30 @@
package com.commafeed.backend; package com.commafeed.backend;
import java.io.IOException; import java.io.IOException;
import java.net.CookieHandler; import java.util.ArrayList;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpClient.Redirect;
import java.net.http.HttpClient.Version;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.time.Duration;
import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.Map;
import java.util.Optional; import java.util.Optional;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.StringUtils;
import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.NameValuePair;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.message.BasicHeader;
import org.apache.http.util.EntityUtils;
import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpStatus;
import com.commafeed.CommaFeedConfiguration; import com.commafeed.CommaFeedConfiguration;
import com.google.common.collect.Iterables;
import com.google.common.net.HttpHeaders; import com.google.common.net.HttpHeaders;
import io.dropwizard.lifecycle.Managed;
import jakarta.inject.Inject; import jakarta.inject.Inject;
import jakarta.inject.Singleton; import jakarta.inject.Singleton;
import lombok.Getter; import lombok.Getter;
@@ -31,24 +36,15 @@ import nl.altindag.ssl.SSLFactory;
* *
*/ */
@Singleton @Singleton
public class HttpGetter { public class HttpGetter implements Managed {
static { private final CloseableHttpClient client;
// reduce connection keepalive timeout to 30s, default is 20 minutes
// https://stackoverflow.com/a/53620696/1885506
// will no longer be needed with Java 21+
// https://bugs.openjdk.org/browse/JDK-8297030
System.setProperty("jdk.httpclient.keepalive.timeout", "30");
}
private final HttpClient client;
private final String userAgent;
@Inject @Inject
public HttpGetter(CommaFeedConfiguration config) { public HttpGetter(CommaFeedConfiguration config) {
this.client = newClient(); String userAgent = Optional.ofNullable(config.getApplicationSettings().getUserAgent())
this.userAgent = Optional.ofNullable(config.getApplicationSettings().getUserAgent())
.orElseGet(() -> String.format("CommaFeed/%s (https://github.com/Athou/commafeed)", config.getVersion())); .orElseGet(() -> String.format("CommaFeed/%s (https://github.com/Athou/commafeed)", config.getVersion()));
this.client = newClient(userAgent, config.getApplicationSettings().getBackgroundThreads());
} }
public HttpResult getBinary(String url, int timeout) throws IOException, NotModifiedException, InterruptedException { public HttpResult getBinary(String url, int timeout) throws IOException, NotModifiedException, InterruptedException {
@@ -70,70 +66,77 @@ public class HttpGetter {
throws IOException, NotModifiedException, InterruptedException { throws IOException, NotModifiedException, InterruptedException {
long start = System.currentTimeMillis(); long start = System.currentTimeMillis();
HttpRequest.Builder builder = HttpRequest.newBuilder() HttpGet request = new HttpGet(url);
.uri(URI.create(url))
.timeout(Duration.ofMillis(timeout))
.header(HttpHeaders.ACCEPT_LANGUAGE, "en")
.header(HttpHeaders.PRAGMA, "No-cache")
.header(HttpHeaders.CACHE_CONTROL, "no-cache")
.header(HttpHeaders.USER_AGENT, userAgent);
if (lastModified != null) { if (lastModified != null) {
builder.header(HttpHeaders.IF_MODIFIED_SINCE, lastModified); request.addHeader(HttpHeaders.IF_MODIFIED_SINCE, lastModified);
} }
if (eTag != null) { if (eTag != null) {
builder.header(HttpHeaders.IF_NONE_MATCH, eTag); request.addHeader(HttpHeaders.IF_NONE_MATCH, eTag);
}
HttpRequest request = builder.GET().build();
HttpResponse<byte[]> response = client.send(request, HttpResponse.BodyHandlers.ofByteArray());
int code = response.statusCode();
if (code == HttpStatus.NOT_MODIFIED_304) {
throw new NotModifiedException("'304 - not modified' http code received");
} else if (code >= 300) {
throw new HttpResponseException(code, "Server returned HTTP error code " + code);
} }
String lastModifiedHeader = response.headers().firstValue(HttpHeaders.LAST_MODIFIED).map(StringUtils::trimToNull).orElse(null); HttpClientContext context = HttpClientContext.create();
if (lastModifiedHeader != null && lastModifiedHeader.equals(lastModified)) { context.setRequestConfig(
throw new NotModifiedException("lastModifiedHeader is the same"); RequestConfig.custom().setConnectTimeout(timeout).setConnectionRequestTimeout(timeout).setSocketTimeout(timeout).build());
try (CloseableHttpResponse response = client.execute(request, context)) {
int code = response.getStatusLine().getStatusCode();
if (code == HttpStatus.NOT_MODIFIED_304) {
throw new NotModifiedException("'304 - not modified' http code received");
} else if (code >= 300) {
throw new HttpResponseException(code, "Server returned HTTP error code " + code);
}
String lastModifiedHeader = Optional.ofNullable(response.getFirstHeader(HttpHeaders.LAST_MODIFIED))
.map(NameValuePair::getValue)
.map(StringUtils::trimToNull)
.orElse(null);
if (lastModifiedHeader != null && lastModifiedHeader.equals(lastModified)) {
throw new NotModifiedException("lastModifiedHeader is the same");
}
String eTagHeader = Optional.ofNullable(response.getFirstHeader(HttpHeaders.ETAG))
.map(NameValuePair::getValue)
.map(StringUtils::trimToNull)
.orElse(null);
if (eTagHeader != null && eTagHeader.equals(eTag)) {
throw new NotModifiedException("eTagHeader is the same");
}
HttpEntity entity = response.getEntity();
byte[] content = entity == null ? null : EntityUtils.toByteArray(entity);
String contentType = Optional.ofNullable(entity).map(HttpEntity::getContentType).map(Header::getValue).orElse(null);
String urlAfterRedirect = CollectionUtils.isEmpty(context.getRedirectLocations()) ? url
: Iterables.getLast(context.getRedirectLocations()).toString();
long duration = System.currentTimeMillis() - start;
return new HttpResult(content, contentType, lastModifiedHeader, eTagHeader, duration, urlAfterRedirect);
} }
String eTagHeader = response.headers().firstValue(HttpHeaders.ETAG).map(StringUtils::trimToNull).orElse(null);
if (eTagHeader != null && eTagHeader.equals(eTag)) {
throw new NotModifiedException("eTagHeader is the same");
}
byte[] content = response.body();
String contentType = response.headers().firstValue(HttpHeaders.CONTENT_TYPE).orElse(null);
String urlAfterRedirect = response.request().uri().toString();
long duration = System.currentTimeMillis() - start;
return new HttpResult(content, contentType, lastModifiedHeader, eTagHeader, duration, urlAfterRedirect);
} }
private HttpClient newClient() { private CloseableHttpClient newClient(String userAgent, int poolSize) {
SSLFactory sslFactory = SSLFactory.builder().withUnsafeTrustMaterial().withUnsafeHostnameVerifier().build(); SSLFactory sslFactory = SSLFactory.builder().withUnsafeTrustMaterial().withUnsafeHostnameVerifier().build();
return HttpClient.newBuilder()
.version(Version.HTTP_1_1) List<Header> headers = new ArrayList<>();
.connectTimeout(Duration.ofSeconds(5)) headers.add(new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en"));
.followRedirects(Redirect.ALWAYS) headers.add(new BasicHeader(HttpHeaders.PRAGMA, "No-cache"));
.sslContext(sslFactory.getSslContext()) headers.add(new BasicHeader(HttpHeaders.CACHE_CONTROL, "no-cache"));
.sslParameters(sslFactory.getSslParameters())
.cookieHandler(new IgnoreCookieHandler()) return HttpClientBuilder.create()
.useSystemProperties()
.disableAutomaticRetries()
.disableCookieManagement()
.setUserAgent(userAgent)
.setDefaultHeaders(headers)
.setSSLContext(sslFactory.getSslContext())
.setSSLHostnameVerifier(sslFactory.getHostnameVerifier())
.setMaxConnTotal(poolSize)
.setMaxConnPerRoute(poolSize)
.build(); .build();
} }
private static class IgnoreCookieHandler extends CookieHandler { @Override
@Override public void stop() throws Exception {
public Map<String, List<String>> get(URI uri, Map<String, List<String>> requestHeaders) { client.close();
return Collections.emptyMap();
}
@Override
public void put(URI uri, Map<String, List<String>> responseHeaders) {
// do nothing
}
} }
@Getter @Getter

View File

@@ -1,12 +1,12 @@
package com.commafeed.backend; package com.commafeed.backend;
import java.io.IOException; import java.io.IOException;
import java.net.http.HttpConnectTimeoutException; import java.net.SocketTimeoutException;
import java.net.http.HttpTimeoutException;
import java.util.Objects; import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicInteger;
import org.apache.commons.io.IOUtils; import org.apache.commons.io.IOUtils;
import org.apache.http.conn.ConnectTimeoutException;
import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpStatus;
import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.BeforeEach;
@@ -87,6 +87,10 @@ class HttpGetterTest {
@Test @Test
void followRedirects() throws Exception { void followRedirects() throws Exception {
this.mockServerClient.when(HttpRequest.request().withMethod("GET").withPath("/redirected")) this.mockServerClient.when(HttpRequest.request().withMethod("GET").withPath("/redirected"))
.respond(HttpResponse.response()
.withStatusCode(HttpStatus.MOVED_TEMPORARILY_302)
.withHeader(HttpHeaders.LOCATION, "http://localhost:" + this.mockServerClient.getPort() + "/redirected-2"));
this.mockServerClient.when(HttpRequest.request().withMethod("GET").withPath("/redirected-2"))
.respond(HttpResponse.response().withBody(feedContent).withContentType(MediaType.APPLICATION_ATOM_XML)); .respond(HttpResponse.response().withBody(feedContent).withContentType(MediaType.APPLICATION_ATOM_XML));
this.mockServerClient.when(HttpRequest.request().withMethod("GET")) this.mockServerClient.when(HttpRequest.request().withMethod("GET"))
.respond(HttpResponse.response() .respond(HttpResponse.response()
@@ -94,26 +98,23 @@ class HttpGetterTest {
.withHeader(HttpHeaders.LOCATION, "http://localhost:" + this.mockServerClient.getPort() + "/redirected")); .withHeader(HttpHeaders.LOCATION, "http://localhost:" + this.mockServerClient.getPort() + "/redirected"));
HttpResult result = getter.getBinary(this.feedUrl, TIMEOUT); HttpResult result = getter.getBinary(this.feedUrl, TIMEOUT);
Assertions.assertEquals("http://localhost:" + this.mockServerClient.getPort() + "/redirected", result.getUrlAfterRedirect()); Assertions.assertEquals("http://localhost:" + this.mockServerClient.getPort() + "/redirected-2", result.getUrlAfterRedirect());
} }
@Test @Test
void timeout() { void dataTimeout() {
int smallTimeout = 500; int smallTimeout = 500;
this.mockServerClient.when(HttpRequest.request().withMethod("GET")) this.mockServerClient.when(HttpRequest.request().withMethod("GET"))
.respond(HttpResponse.response().withDelay(Delay.milliseconds(smallTimeout * 2))); .respond(HttpResponse.response().withDelay(Delay.milliseconds(smallTimeout * 2)));
HttpTimeoutException e = Assertions.assertThrows(HttpTimeoutException.class, () -> getter.getBinary(this.feedUrl, smallTimeout)); Assertions.assertThrows(SocketTimeoutException.class, () -> getter.getBinary(this.feedUrl, smallTimeout));
Assertions.assertEquals("request timed out", e.getMessage());
} }
@Test @Test
void connectTimeout() { void connectTimeout() {
// try to connect to a non-routable address // try to connect to a non-routable address
// https://stackoverflow.com/a/904609/1885506 // https://stackoverflow.com/a/904609/1885506
HttpConnectTimeoutException e = Assertions.assertThrows(HttpConnectTimeoutException.class, Assertions.assertThrows(ConnectTimeoutException.class, () -> getter.getBinary("http://10.255.255.1", 2000));
() -> getter.getBinary("http://10.255.255.1", 10000));
Assertions.assertEquals("HTTP connect timed out", e.getMessage());
} }
@Test @Test