From deb31c7b09bcff16aab830eafa88ef27a915a852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20L=C3=A4ubrich?= Date: Fri, 9 Feb 2024 12:03:51 +0100 Subject: [PATCH] Improve GOAWAY handling Currently GOAWAY is detected/handled only in the connection phase but sometimes can happen in the transfer phase as well. This now wraps the whole request inside a block that is retried if a GOAWAY is detected. --- .../tycho/p2maven/transport/Headers.java | 59 ++++++++ .../p2maven/transport/HttpTransport.java | 7 +- .../transport/Java11HttpTransportFactory.java | 141 +++++++++++------- .../tycho/p2maven/transport/Response.java | 51 +++---- .../transport/SharedHttpCacheStorage.java | 65 +++----- .../transport/URLHttpTransportFactory.java | 29 ++-- 6 files changed, 212 insertions(+), 140 deletions(-) create mode 100644 p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Headers.java diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Headers.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Headers.java new file mode 100644 index 0000000000..b073308f4b --- /dev/null +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Headers.java @@ -0,0 +1,59 @@ +/******************************************************************************* + * Copyright (c) 2024 Christoph Läubrich and others. + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Christoph Läubrich - initial API and implementation + *******************************************************************************/ +package org.eclipse.tycho.p2maven.transport; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.URI; +import java.util.List; +import java.util.Map; + +public interface Headers extends AutoCloseable { + + String ENCODING_IDENTITY = "identity"; + String HEADER_ACCEPT_ENCODING = "Accept-Encoding"; + String HEADER_CONTENT_ENCODING = "Content-Encoding"; + String ENCODING_GZIP = "gzip"; + String ETAG_HEADER = "ETag"; + String LAST_MODIFIED_HEADER = "Last-Modified"; + String EXPIRES_HEADER = "Expires"; + String CACHE_CONTROL_HEADER = "Cache-Control"; + String MAX_AGE_DIRECTIVE = "max-age"; + String MUST_REVALIDATE_DIRECTIVE = "must-revalidate"; + + int statusCode() throws IOException; + + Map> headers(); + + @Override + void close(); + + URI getURI(); + + String getHeader(String header); + + long getLastModified(); + + default void checkResponseCode() throws FileNotFoundException, IOException { + int code = statusCode(); + if (code >= HttpURLConnection.HTTP_BAD_REQUEST) { + if (code == HttpURLConnection.HTTP_NOT_FOUND || code == HttpURLConnection.HTTP_GONE) { + throw new FileNotFoundException(getURI().toString()); + } else { + throw new java.io.IOException("Server returned HTTP code: " + code + " for URL " + getURI().toString()); + } + } + } + +} diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/HttpTransport.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/HttpTransport.java index 4a52f21a7d..0dfeb1da41 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/HttpTransport.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/HttpTransport.java @@ -1,14 +1,15 @@ package org.eclipse.tycho.p2maven.transport; import java.io.IOException; -import java.io.InputStream; + +import org.eclipse.tycho.p2maven.transport.Response.ResponseConsumer; public interface HttpTransport { void setHeader(String key, String value); - Response get() throws IOException; + T get(ResponseConsumer consumer) throws IOException; - Response head() throws IOException; + Headers head() throws IOException; } diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Java11HttpTransportFactory.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Java11HttpTransportFactory.java index 22d33f8576..1d6ca5a0f7 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Java11HttpTransportFactory.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Java11HttpTransportFactory.java @@ -15,6 +15,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InterruptedIOException; +import java.io.OutputStream; import java.net.Proxy; import java.net.ProxySelector; import java.net.SocketAddress; @@ -42,6 +43,7 @@ import org.codehaus.plexus.personality.plexus.lifecycle.phase.Initializable; import org.codehaus.plexus.personality.plexus.lifecycle.phase.InitializationException; import org.eclipse.tycho.p2maven.helper.ProxyHelper; +import org.eclipse.tycho.p2maven.transport.Response.ResponseConsumer; /** * A transport using Java11 HttpClient @@ -77,7 +79,7 @@ public class Java11HttpTransportFactory implements HttpTransportFactory, Initial @Override public HttpTransport createTransport(URI uri) { Java11HttpTransport transport = new Java11HttpTransport(client, clientHttp1, HttpRequest.newBuilder().uri(uri), - logger); + uri, logger); authenticator.preemtiveAuth((k, v) -> transport.setHeader(k, v), uri); return transport; } @@ -88,11 +90,13 @@ private static final class Java11HttpTransport implements HttpTransport { private HttpClient client; private Logger logger; private HttpClient clientHttp1; + private URI uri; - public Java11HttpTransport(HttpClient client, HttpClient clientHttp1, Builder builder, Logger logger) { + public Java11HttpTransport(HttpClient client, HttpClient clientHttp1, Builder builder, URI uri, Logger logger) { this.client = client; this.clientHttp1 = clientHttp1; this.builder = builder; + this.uri = uri; this.logger = logger; } @@ -102,78 +106,106 @@ public void setHeader(String key, String value) { } @Override - public Response get() throws IOException { + public T get(ResponseConsumer consumer) throws IOException { try { - HttpResponse response = performGet(); - return new ResponseImplementation<>(response) { - - @Override - public void close() { - if (response.version() == Version.HTTP_1_1) { - // discard any remaining data and close the stream to return the connection to - // the pool.. - try (InputStream stream = body()) { - int discarded = 0; - while (discarded < MAX_DISCARD) { - int read = stream.read(DUMMY_BUFFER); - if (read < 0) { - break; - } - discarded += read; - } - } catch (IOException e) { - // don't care... - } - } else { - // just closing should be enough to signal to the framework... - try (InputStream stream = body()) { - } catch (IOException e) { - // don't care... - } - } + try { + return performGet(consumer, client); + } catch (IOException e) { + if (isGoaway(e)) { + logger.info("Received GOAWAY from server " + uri.getHost() + " will retry with Http/1..."); + TimeUnit.SECONDS.sleep(1); + return performGet(consumer, clientHttp1); } - }; + throw e; + } } catch (InterruptedException e) { throw new InterruptedIOException(); } } - private HttpResponse performGet() throws IOException, InterruptedException { + private T performGet(ResponseConsumer consumer, HttpClient httpClient) + throws IOException, InterruptedException { HttpRequest request = builder.GET().timeout(Duration.ofSeconds(TIMEOUT_SECONDS)).build(); - try { - return client.send(request, BodyHandlers.ofInputStream()); - } catch (IOException e) { - if (isGoaway(e)) { - logger.warn("Received GOAWAY from server " + request.uri().getHost() - + " will retry after one second with Http/1..."); - TimeUnit.SECONDS.sleep(1); - return clientHttp1.send(request, BodyHandlers.ofInputStream()); + HttpResponse response = httpClient.send(request, BodyHandlers.ofInputStream()); + try (ResponseImplementation implementation = new ResponseImplementation<>(response) { + + @Override + public void close() { + if (response.version() == Version.HTTP_1_1) { + // discard any remaining data and close the stream to return the connection to + // the pool.. + try (InputStream stream = response.body()) { + int discarded = 0; + while (discarded < MAX_DISCARD) { + int read = stream.read(DUMMY_BUFFER); + if (read < 0) { + break; + } + discarded += read; + } + } catch (IOException e) { + // don't care... + } + } else { + // just closing should be enough to signal to the framework... + try (InputStream stream = response.body()) { + } catch (IOException e) { + // don't care... + } + } } - throw e; + + @Override + public void transferTo(OutputStream outputStream, ContentEncoding transportEncoding) + throws IOException { + transportEncoding.decode(response.body()).transferTo(outputStream); + } + }) { + return consumer.handleResponse(implementation); } } @Override - public Response head() throws IOException { + public Response head() throws IOException { try { - HttpResponse response = client.send( - builder.method("HEAD", BodyPublishers.noBody()).timeout(Duration.ofSeconds(TIMEOUT_SECONDS)) - .build(), - BodyHandlers.discarding()); - return new ResponseImplementation<>(response) { - @Override - public void close() { - // nothing... + try { + return doHead(client); + } catch (IOException e) { + if (isGoaway(e)) { + logger.debug("Received GOAWAY from server " + uri.getHost() + + " will retry with Http/1..."); + TimeUnit.SECONDS.sleep(1); + return doHead(clientHttp1); } - }; + throw e; + } } catch (InterruptedException e) { throw new InterruptedIOException(); } } + private Response doHead(HttpClient httpClient) throws IOException, InterruptedException { + HttpResponse response = httpClient.send( + builder.method("HEAD", BodyPublishers.noBody()).timeout(Duration.ofSeconds(TIMEOUT_SECONDS)) + .build(), + BodyHandlers.discarding()); + return new ResponseImplementation<>(response) { + @Override + public void close() { + // nothing... + } + + @Override + public void transferTo(OutputStream outputStream, ContentEncoding transportEncoding) + throws IOException { + throw new IOException("HEAD returns no body"); + } + }; + } + } - private static abstract class ResponseImplementation implements Response { + private static abstract class ResponseImplementation implements Response { private final HttpResponse response; private ResponseImplementation(HttpResponse response) { @@ -190,11 +222,6 @@ public Map> headers() { return response.headers().map(); } - @Override - public T body() { - return response.body(); - } - @Override public String getHeader(String header) { return response.headers().firstValue(header).orElse(null); diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Response.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Response.java index 94564f1d7f..088846f0de 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Response.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/Response.java @@ -12,39 +12,32 @@ *******************************************************************************/ package org.eclipse.tycho.p2maven.transport; -import java.io.FileNotFoundException; import java.io.IOException; -import java.net.HttpURLConnection; -import java.net.URI; -import java.util.List; -import java.util.Map; - -public interface Response extends AutoCloseable { - - int statusCode() throws IOException; - - Map> headers(); - - @Override - void close(); - - T body() throws IOException; - - URI getURI(); +import java.io.InputStream; +import java.io.OutputStream; +import java.util.zip.GZIPInputStream; + +public interface Response extends Headers { + + default void transferTo(OutputStream outputStream) throws IOException { + String encoding = getHeader(Headers.HEADER_CONTENT_ENCODING); + if (Headers.ENCODING_GZIP.equals(encoding)) { + transferTo(outputStream, GZIPInputStream::new); + } else if (encoding == null || encoding.isEmpty() || Headers.ENCODING_IDENTITY.equals(encoding)) { + transferTo(outputStream, stream -> stream); + } else { + throw new IOException("Unknown content encoding: " + encoding); + } + } - String getHeader(String header); + void transferTo(OutputStream outputStream, ContentEncoding transportEncoding) throws IOException; - long getLastModified(); + interface ContentEncoding { + InputStream decode(InputStream raw) throws IOException; + } - default void checkResponseCode() throws FileNotFoundException, IOException { - int code = statusCode(); - if (code >= HttpURLConnection.HTTP_BAD_REQUEST) { - if (code == HttpURLConnection.HTTP_NOT_FOUND || code == HttpURLConnection.HTTP_GONE) { - throw new FileNotFoundException(getURI().toString()); - } else { - throw new java.io.IOException("Server returned HTTP code: " + code + " for URL " + getURI().toString()); - } - } + interface ResponseConsumer { + T handleResponse(Response response) throws IOException; } } diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java index 32237fc0e8..bd173b2153 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java @@ -17,7 +17,6 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; -import java.io.InputStream; import java.net.HttpURLConnection; import java.net.URI; import java.text.DateFormat; @@ -33,7 +32,6 @@ import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.stream.Collectors; -import java.util.zip.GZIPInputStream; import org.apache.commons.io.FileUtils; import org.codehaus.plexus.component.annotations.Component; @@ -51,14 +49,6 @@ public class SharedHttpCacheStorage implements HttpCache { // TODO can we sync this with the time where maven updates snapshots? public static final long MIN_CACHE_PERIOD = Long.getLong("tycho.p2.transport.min-cache-minutes", TimeUnit.HOURS.toMinutes(1)); - private static final String LAST_MODIFIED_HEADER = "Last-Modified"; - private static final String EXPIRES_HEADER = "Expires"; - private static final String CACHE_CONTROL_HEADER = "Cache-Control"; - private static final String MAX_AGE_DIRECTIVE = "max-age"; - private static final String MUST_REVALIDATE_DIRECTIVE = "must-revalidate"; - - private static final String ETAG_HEADER = "ETag"; - private static final int MAX_IN_MEMORY = 1000; @Requirement @@ -168,10 +158,6 @@ private synchronized CacheLine getCacheLine(URI uri) { private final class CacheLine { - private static final String ENCODING_IDENTITY = "identity"; - private static final String HEADER_ACCEPT_ENCODING = "Accept-Encoding"; - private static final String HEADER_CONTENT_ENCODING = "Content-Encoding"; - private static final String ENCODING_GZIP = "gzip"; private static final String RESPONSE_CODE = "HTTP_RESPONSE_CODE"; private static final String LAST_UPDATED = "FILE-LAST_UPDATED"; private static final String STATUS_LINE = "HTTP_STATUS_LINE"; @@ -192,7 +178,7 @@ public synchronized long fetchLastModified(URI uri, HttpTransportFactory transpo // probably just download it right now? HttpTransport transport = transportFactory.createTransport(uri); - try (Response response = transport.head()) { + try (Headers response = transport.head()) { int code = response.statusCode(); if (isAuthFailure(code)) { throw new AuthenticationFailedException(); // FIXME why is there no constructor to give a cause? @@ -223,7 +209,7 @@ public synchronized long getLastModified(URI uri, HttpTransportFactory transport return SharedHttpCacheStorage.this.getCacheEntry(uri, logger).getLastModified(transportFactory); } Properties offlineHeader = getHeader(); - Date lastModified = pareHttpDate(offlineHeader.getProperty(LAST_MODIFIED_HEADER.toLowerCase())); + Date lastModified = pareHttpDate(offlineHeader.getProperty(Headers.LAST_MODIFIED_HEADER.toLowerCase())); if (lastModified != null) { return lastModified.getTime(); } @@ -242,16 +228,17 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor HttpTransport transport = transportFactory.createTransport(uri); Properties lastHeader = getHeader(); if (exits) { - if (lastHeader.containsKey(ETAG_HEADER.toLowerCase())) { - transport.setHeader("If-None-Match", lastHeader.getProperty(ETAG_HEADER.toLowerCase())); + if (lastHeader.containsKey(Headers.ETAG_HEADER.toLowerCase())) { + transport.setHeader("If-None-Match", lastHeader.getProperty(Headers.ETAG_HEADER.toLowerCase())); } - if (lastHeader.contains(LAST_MODIFIED_HEADER.toLowerCase())) { + if (lastHeader.contains(Headers.LAST_MODIFIED_HEADER.toLowerCase())) { transport.setHeader("If-Modified-Since", - lastHeader.getProperty(LAST_MODIFIED_HEADER.toLowerCase())); + lastHeader.getProperty(Headers.LAST_MODIFIED_HEADER.toLowerCase())); } } - transport.setHeader(HEADER_ACCEPT_ENCODING, ENCODING_GZIP); - try (Response response = transport.get()) { + transport.setHeader(Headers.HEADER_ACCEPT_ENCODING, Headers.ENCODING_GZIP); + return transport.get(response -> { + File tempFile; int code = response.statusCode(); if (exits && code == HttpURLConnection.HTTP_NOT_MODIFIED) { updateHeader(response, getResponseCode()); @@ -270,6 +257,7 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor // Copying file to accommodate original request and its file extension. // Once https://github.com/eclipse-equinox/p2/issues/355 is fixed, cachedFile // may be returned directly without copying. + response.close(); // early close before doing unrelated file I/O FileUtils.copyFile(cachedFile, file); return file; } @@ -277,23 +265,18 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor FileUtils.forceDelete(file); } response.checkResponseCode(); - File tempFile = File.createTempFile("download", ".tmp", file.getParentFile()); - try (InputStream inputStream = response.body(); FileOutputStream os = new FileOutputStream(tempFile)) { - String encoding = response.getHeader(HEADER_CONTENT_ENCODING); - if (ENCODING_GZIP.equals(encoding)) { - new GZIPInputStream(inputStream).transferTo(os); - } else if (encoding == null || encoding.isEmpty() || ENCODING_IDENTITY.equals(encoding)) { - inputStream.transferTo(os); - } else { - throw new IOException("Unknown content encoding: " + encoding); - } + tempFile = File.createTempFile("download", ".tmp", file.getParentFile()); + try (FileOutputStream os = new FileOutputStream(tempFile)) { + response.transferTo(os); } catch (IOException e) { tempFile.delete(); throw e; } + response.close(); // early close before doing file I/O FileUtils.moveFile(tempFile, file); - } - return file; + return file; + }); + } public synchronized File getFile(URI uri, HttpTransportFactory transportFactory, @@ -324,7 +307,7 @@ private boolean mustValidate() { } String[] cacheControls = getCacheControl(); for (String directive : cacheControls) { - if (MUST_REVALIDATE_DIRECTIVE.equals(directive)) { + if (Headers.MUST_REVALIDATE_DIRECTIVE.equals(directive)) { // server enforced validation return true; } @@ -337,15 +320,15 @@ private boolean mustValidate() { // Cache-Control header with "max-age" directive takes precedence over Expires // Header. for (String directive : cacheControls) { - if (directive.toLowerCase().startsWith(MAX_AGE_DIRECTIVE)) { - long maxAge = parseLong(directive.substring(MAX_AGE_DIRECTIVE.length() + 1)); + if (directive.toLowerCase().startsWith(Headers.MAX_AGE_DIRECTIVE)) { + long maxAge = parseLong(directive.substring(Headers.MAX_AGE_DIRECTIVE.length() + 1)); if (maxAge <= 0) { return true; } return (lastUpdated + TimeUnit.SECONDS.toMillis(maxAge)) < System.currentTimeMillis(); } } - Date expiresDate = pareHttpDate(properties.getProperty(EXPIRES_HEADER.toLowerCase())); + Date expiresDate = pareHttpDate(properties.getProperty(Headers.EXPIRES_HEADER.toLowerCase())); if (expiresDate != null) { return expiresDate.after(new Date()); } @@ -364,7 +347,7 @@ protected long parseLong(String value) { } private String[] getCacheControl() { - String property = getHeader().getProperty(CACHE_CONTROL_HEADER); + String property = getHeader().getProperty(Headers.CACHE_CONTROL_HEADER); if (property != null) { return property.split(",\\s*"); } @@ -375,7 +358,7 @@ protected boolean isAuthFailure(int code) { return code == HttpURLConnection.HTTP_PROXY_AUTH || code == HttpURLConnection.HTTP_UNAUTHORIZED; } - protected void updateHeader(Response response, int code) throws IOException, FileNotFoundException { + protected void updateHeader(Headers response, int code) throws IOException, FileNotFoundException { header = new Properties(); header.setProperty(RESPONSE_CODE, String.valueOf(code)); header.setProperty(LAST_UPDATED, String.valueOf(System.currentTimeMillis())); @@ -395,7 +378,7 @@ protected void updateHeader(Response response, int code) throws IOException, // don't store non default header... continue; } - if (HEADER_CONTENT_ENCODING.equals(key)) { + if (Headers.HEADER_CONTENT_ENCODING.equals(key)) { // we already decode the content before... continue; } diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/URLHttpTransportFactory.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/URLHttpTransportFactory.java index 3b98137aac..a580abb56f 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/URLHttpTransportFactory.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/URLHttpTransportFactory.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.io.InputStream; +import java.io.OutputStream; import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URI; @@ -25,6 +26,7 @@ import org.codehaus.plexus.component.annotations.Component; import org.codehaus.plexus.component.annotations.Requirement; import org.eclipse.tycho.p2maven.helper.ProxyHelper; +import org.eclipse.tycho.p2maven.transport.Response.ResponseConsumer; @Component(role = HttpTransportFactory.class, hint = URLHttpTransportFactory.HINT) public class URLHttpTransportFactory implements HttpTransportFactory { @@ -60,10 +62,10 @@ public void setHeader(String key, String value) { } @Override - public Response get() throws IOException { + public T get(ResponseConsumer consumer) throws IOException { HttpURLConnection connection = createConnection(); connection.connect(); - return new HttpResponse<>(connection) { + try (HttpResponse response = new HttpResponse(connection) { @Override public void close() { @@ -90,11 +92,16 @@ private InputStream anyBody() throws IOException { } @Override - public InputStream body() throws IOException { - return connection.getInputStream(); + public void transferTo(OutputStream outputStream, ContentEncoding transportEncoding) + throws IOException { + transportEncoding.decode(connection.getInputStream()).transferTo(outputStream); + } - }; + + }) { + return consumer.handleResponse(response); + } } private HttpURLConnection createConnection() throws IOException, MalformedURLException { @@ -107,11 +114,11 @@ private HttpURLConnection createConnection() throws IOException, MalformedURLExc } @Override - public Response head() throws IOException { + public Response head() throws IOException { HttpURLConnection connection = createConnection(); connection.setRequestMethod("HEAD"); connection.connect(); - return new HttpResponse<>(connection) { + return new HttpResponse(connection) { @Override public void close() { @@ -119,14 +126,16 @@ public void close() { } @Override - public Void body() throws IOException { - return null; + public void transferTo(OutputStream outputStream, ContentEncoding transportEncoding) + throws IOException { + throw new IOException("Only headers!"); } + }; } } - private static abstract class HttpResponse implements Response { + private static abstract class HttpResponse implements Response { private HttpURLConnection connection;