From 8462a37d62b112b7ff9f40e519c066357fe5808d Mon Sep 17 00:00:00 2001 From: Patrick Ziegler Date: Sat, 14 Sep 2024 08:55:32 +0200 Subject: [PATCH] Improve errors handling while downloading p2 artifacts from repository In case an artifact couldn't be downloaded due to e.g. a bad mirror, p2 returns an RETRY error code. This code is currently ignored and Tycho simply fails with an I/O exception. Instead, Tycho attempts to download the artifact up to three times (assuming that this error code was returned), before failing. This value can be configured using the 'eclipse.p2.maxDownloadAttempts' system property. --- .../repository/P2RepositoryManager.java | 43 ++++- src/site/markdown/SystemProperties.md | 5 +- .../baseline/artifacts.xml | 20 +++ .../p2Repository.mirror/baseline/content.xml | 34 ++++ .../p2Repository.mirror/baseline/mirrors.xml | 4 + .../baseline/plugins/bundle1_1.0.0.jar | Bin 0 -> 417 bytes .../bundle/META-INF/MANIFEST.MF | 6 + .../bundle/build.properties | 1 + .../p2Repository.mirror/bundle/pom.xml | 45 +++++ .../projects/p2Repository.mirror/pom.xml | 23 +++ .../p2Repository/P2RepositoryMirrorTest.java | 160 ++++++++++++++++++ .../eclipse/tycho/test/util/HttpServer.java | 7 +- 12 files changed, 339 insertions(+), 9 deletions(-) create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/content.xml create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/plugins/bundle1_1.0.0.jar create mode 100644 tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF create mode 100644 tycho-its/projects/p2Repository.mirror/bundle/build.properties create mode 100644 tycho-its/projects/p2Repository.mirror/bundle/pom.xml create mode 100644 tycho-its/projects/p2Repository.mirror/pom.xml create mode 100644 tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java index 18f7cdfcb4..f6a7e056bc 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2022 Christoph Läubrich and others. + * Copyright (c) 2022, 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 @@ -37,6 +37,7 @@ import org.eclipse.equinox.p2.repository.metadata.IMetadataRepositoryManager; import org.eclipse.tycho.IRepositoryIdManager; import org.eclipse.tycho.MavenRepositoryLocation; +import org.eclipse.tycho.helper.MavenPropertyHelper; import org.eclipse.tycho.p2maven.ListCompositeArtifactRepository; import org.eclipse.tycho.p2maven.ListQueryable; import org.eclipse.tycho.p2maven.LoggerProgressMonitor; @@ -46,6 +47,10 @@ */ @Component(role = P2RepositoryManager.class) public class P2RepositoryManager { + private static final String PROPERTY_KEY = "eclipse.p2.maxDownloadAttempts"; + + @Requirement + MavenPropertyHelper propertyHelper; @Requirement IRepositoryIdManager repositoryIdManager; @@ -151,18 +156,44 @@ private IMetadataRepository getMetadataRepositor(URI location, String id) throws public void downloadArtifact(IInstallableUnit iu, IArtifactRepository artifactRepository, OutputStream outputStream) throws IOException { Collection artifacts = iu.getArtifacts(); + int maxDownloadAttempts = getMaxDownloadAttempts(); + for (IArtifactKey key : artifacts) { IArtifactDescriptor[] descriptors = artifactRepository.getArtifactDescriptors(key); for (IArtifactDescriptor descriptor : descriptors) { - IStatus status = artifactRepository.getRawArtifact(descriptor, outputStream, - new LoggerProgressMonitor(logger)); - if (status.isOK()) { - return; + for (int downloadAttempts = 0; downloadAttempts < maxDownloadAttempts; ++downloadAttempts) { + IStatus status = artifactRepository.getRawArtifact(descriptor, outputStream, + new LoggerProgressMonitor(logger)); + if (status.isOK()) { + return; + } + // Might happen if e.g. a bad mirror was used + if (status.getCode() == IArtifactRepository.CODE_RETRY) { + logger.warn("Artifact repository requested retry (attempt [%d/%d]): '%s'" + .formatted(downloadAttempts + 1, maxDownloadAttempts, status)); + continue; + } + throw new IOException("Download failed: " + status); } - throw new IOException("Download failed: " + status); } } throw new FileNotFoundException(); } + private int getMaxDownloadAttempts() { + String property = propertyHelper.getGlobalProperty(PROPERTY_KEY, "3"); + try { + int maxDownloadAttempts = Integer.valueOf(property); + if (maxDownloadAttempts <= 0) { + logger.error("Value '%s' for property '%s', is not a positive number! Use 1 as default value." + .formatted(property, PROPERTY_KEY)); + return 1; + } + return maxDownloadAttempts; + } catch (NumberFormatException e) { + logger.error("Value '%s' for property '%s', is not a number! Use 1 as default value.".formatted(property, + PROPERTY_KEY)); + return 1; + } + } } diff --git a/src/site/markdown/SystemProperties.md b/src/site/markdown/SystemProperties.md index 7ba6c87471..d89a3e3dc9 100644 --- a/src/site/markdown/SystemProperties.md +++ b/src/site/markdown/SystemProperties.md @@ -23,7 +23,7 @@ tycho.debug.resolver | `true` or _artifactId_ | Enable debug output for the arti ### Baseline compare Name | Value | Default | Documentation ---- | --- | --- +--- | --- | --- | --- tycho.comparator.showDiff | true / false | false | If set to true if text-like files show a unified diff of possible differences in files tycho.comparator.threshold | bytes | 5242880 (~5MB) | gives the number of bytes for content to be compared semantically, larger files will only be compared byte-by-byte @@ -32,6 +32,7 @@ tycho.comparator.threshold | bytes | 5242880 (~5MB) | gives the number of bytes These properties control the behaviour of P2 used by Tycho Name | Value | Default | Documentation ---- | --- | --- +--- | --- | --- | --- eclipse.p2.mirrors | true / false | true | Each p2 site can define a list of artifact repository mirrors, this controls if P2 mirrors should be used. This is independent from configuring mirrors in the maven configuration to be used by Tycho! +eclipse.p2.maxDownloadAttempts | _any positive integer_ | 3 | Describes how often Tycho attempts to re-download an artifact from a p2 repository in case e.g. a bad mirror was used. One can think of this value as the maximum number of mirrors Tycho/p2 will check. diff --git a/tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml b/tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml new file mode 100644 index 0000000000..a24132b9d5 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + + diff --git a/tycho-its/projects/p2Repository.mirror/baseline/content.xml b/tycho-its/projects/p2Repository.mirror/baseline/content.xml new file mode 100644 index 0000000000..61f0c290ea --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/baseline/content.xml @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + Bundle-SymbolicName: bundle1 Bundle-Version: 1.0.0 + + + + + + diff --git a/tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml b/tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml new file mode 100644 index 0000000000..79a7bd8601 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/tycho-its/projects/p2Repository.mirror/baseline/plugins/bundle1_1.0.0.jar b/tycho-its/projects/p2Repository.mirror/baseline/plugins/bundle1_1.0.0.jar new file mode 100644 index 0000000000000000000000000000000000000000..529608eb31177c61502765ce706e8d7062a53332 GIT binary patch literal 417 zcmWIWW@h1H0D-dsP9b0hl;C8LVeoYgan$wnbJGtE;bdT*zg}sd?m)3p`_G7H5R)~+$ r+y^lZmwzF~F#-)*(&&tA9E$IOCS&n=fHx}}$O%k9xDrS|0&y4sUcqAN literal 0 HcmV?d00001 diff --git a/tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF b/tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF new file mode 100644 index 0000000000..03efca7aa4 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF @@ -0,0 +1,6 @@ +Manifest-Version: 1.0 +Bundle-ManifestVersion: 2 +Bundle-SymbolicName: bundle1 +Bundle-Version: 1.0.0 +Automatic-Module-Name: bundle1 + diff --git a/tycho-its/projects/p2Repository.mirror/bundle/build.properties b/tycho-its/projects/p2Repository.mirror/bundle/build.properties new file mode 100644 index 0000000000..5f22cdd448 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/bundle/build.properties @@ -0,0 +1 @@ +bin.includes = META-INF/ diff --git a/tycho-its/projects/p2Repository.mirror/bundle/pom.xml b/tycho-its/projects/p2Repository.mirror/bundle/pom.xml new file mode 100644 index 0000000000..fea44d8e91 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/bundle/pom.xml @@ -0,0 +1,45 @@ + + + 4.0.0 + + + p2Repository.mirror + parent + 0.0.1-SNAPSHOT + + + eclipse-plugin + bundle1 + 1.0.0 + + + + + org.eclipse.tycho + tycho-baseline-plugin + ${tycho-version} + + + compare-version-with-baseline + verify + + verify + + + + + + META-INF/maven/**/* + + + + repo + ${baseline} + p2 + + + + + + + diff --git a/tycho-its/projects/p2Repository.mirror/pom.xml b/tycho-its/projects/p2Repository.mirror/pom.xml new file mode 100644 index 0000000000..26040988fa --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/pom.xml @@ -0,0 +1,23 @@ + + 4.0.0 + + p2Repository.mirror + parent + 0.0.1-SNAPSHOT + pom + + + bundle + + + + + + org.eclipse.tycho + tycho-maven-plugin + ${tycho-version} + true + + + + diff --git a/tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java b/tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java new file mode 100644 index 0000000000..807000e158 --- /dev/null +++ b/tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java @@ -0,0 +1,160 @@ +/******************************************************************************* + * Copyright (c) 2024 Patrick Ziegler 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: + * Patrick Ziegler - initial API and implementation + *******************************************************************************/ +package org.eclipse.tycho.test.p2Repository; + +import java.io.File; +import java.io.FileWriter; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; +import javax.xml.xpath.XPath; +import javax.xml.xpath.XPathConstants; +import javax.xml.xpath.XPathFactory; + +import org.apache.maven.it.Verifier; +import org.eclipse.jetty.servlet.DefaultServlet; +import org.eclipse.jetty.util.resource.EmptyResource; +import org.eclipse.jetty.util.resource.Resource; +import org.eclipse.tycho.test.AbstractTychoIntegrationTest; +import org.eclipse.tycho.test.util.HttpServer; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NodeList; + +public class P2RepositoryMirrorTest extends AbstractTychoIntegrationTest { + + private HttpServer server; + + @Before + public void startServer() throws Exception { + server = HttpServer.startServer(); + } + + @After + public void stopServer() throws Exception { + server.stop(); + } + + /** + * Tests whether Tycho is able to recover from a bad mirror repository. If + * multiple mirrors are specified for a repository, Tycho might be able to + * continue by requesting an artifact from a different mirror, depending on the + * error code returned by Equinox. + */ + @Test + public void testMirrorWithRetry() throws Exception { + Verifier verifier = getVerifier("p2Repository.mirror", false); + + String baseline = server.addServer("baseline", FirstBaselineRequestFailsServlet.class, + new File(verifier.getBasedir(), "baseline")); + // Two mirrors, to always have at least one that is still good + String mirror1 = server.addServer("mirror1", FirstBaselineRequestFailsServlet.class, + new File(verifier.getBasedir(), "baseline")); + String mirror2 = server.addServer("mirror2", FirstBaselineRequestFailsServlet.class, + new File(verifier.getBasedir(), "baseline")); + String mirrors = baseline + '/' + "mirrors.xml"; + + setMirrorsUrl(new File(verifier.getBasedir(), "baseline/artifacts.xml"), mirrors); + setMirrors(new File(verifier.getBasedir(), "baseline/mirrors.xml"), mirror1, mirror2); + + // The verifier escapes the 'http://localhost' to 'http:/localhost' + verifier.addCliOption("-Dbaseline=" + baseline.replaceAll("//", "////")); + // Force an update of the HttpCache + verifier.addCliOption("-U"); + verifier.executeGoal("verify"); + verifier.verifyErrorFreeLog(); + verifier.verifyTextInLog("Artifact repository requested retry (attempt [1/3]):"); + } + + @Override + protected boolean isDisableMirrors() { + return false; + } + + /** + * Updates the "p2.mirrorsURL" property in the {@code artifacts.xml} file of the + * baseline repository to point to the {@code mirrors.xml} file. + */ + private static void setMirrorsUrl(File artifactsXml, String mirrorsUrl) throws Exception { + Document document = parseDocument(artifactsXml); + + XPath path = XPathFactory.newInstance().newXPath(); + String expression = "repository/properties/property[@name='p2.mirrorsURL']"; + Element node = (Element) path.evaluate(expression, document, XPathConstants.NODE); + + if (node != null) { + node.setAttribute("value", mirrorsUrl); + writeDocument(document, artifactsXml); + } + } + + /** + * Updates the {@link mirrors.xml} file to contain the bad mirror. + */ + private static void setMirrors(File mirrorsXml, String mirror1, String mirror2) throws Exception { + Document document = parseDocument(mirrorsXml); + + XPath path = XPathFactory.newInstance().newXPath(); + String expression = "mirrors/mirror"; + NodeList nodes = (NodeList) path.evaluate(expression, document, XPathConstants.NODESET); + + if (nodes != null) { + ((Element) nodes.item(0)).setAttribute("url", mirror1); + ((Element) nodes.item(1)).setAttribute("url", mirror2); + writeDocument(document, mirrorsXml); + } + } + + public static Document parseDocument(File file) throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + DocumentBuilder builder = factory.newDocumentBuilder(); + return builder.parse(file); + } + + public static void writeDocument(Document document, File file) throws Exception { + TransformerFactory transformerFactory = TransformerFactory.newInstance(); + Transformer transformer = transformerFactory.newTransformer(); + + try (FileWriter writer = new FileWriter(file)) { + DOMSource source = new DOMSource(document); + StreamResult result = new StreamResult(writer); + transformer.transform(source, result); + } + } + + /** + * Helper servlet to simulate an illegal p2 repository. The first time a plugin + * is requested from the remote repository, a 404 is returned. Any further + * requests succeed, to test whether Tycho is able to recover from bad mirrors. + */ + public static class FirstBaselineRequestFailsServlet extends DefaultServlet { + // We don't know which mirror is selected, so anyone can fail + private static boolean firstRequest = true; + + @Override + public Resource getResource(String pathInContext) { + if (firstRequest && pathInContext.startsWith("/plugins/")) { + firstRequest = false; + return EmptyResource.INSTANCE; + } + return super.getResource(pathInContext); + } + } +} diff --git a/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java b/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java index d22a54bb3f..38e5f46644 100644 --- a/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java +++ b/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java @@ -24,6 +24,7 @@ import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; +import javax.servlet.Servlet; import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; @@ -169,10 +170,14 @@ public void stop() throws Exception { } public String addServer(String contextName, final File content) { + return addServer(contextName, DefaultServlet.class, content); + } + + public String addServer(String contextName, Class servlet, final File content) { ServletContextHandler context = new ServletContextHandler(contexts, URIUtil.SLASH + contextName); context.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false"); context.setResourceBase(content.getAbsolutePath()); - context.addServlet(DefaultServlet.class, URIUtil.SLASH); + context.addServlet(servlet, URIUtil.SLASH); registerContext(context); return getUrl(contextName); }