From ff76d7b0d32c01b381b32bd3d2baa2a754d29750 Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Sat, 21 Oct 2023 23:30:42 +0400 Subject: [PATCH] Fix HttpServer concurrency bug (#2938) URLs returned from HttpServer.getAccessedUrls() are now stripped of context prefix. No callers have used these values until now. Fix concurrency bug in test utility class HttpServer request logging. Add verification of request logs to ensure that metadata repository does indeed unjars fresh, non-cached artifact. Ensure that cached XML file does not produce false negative in tests. Refactor tests. --- .../tycho/test/tycho2938/ContentJarTest.java | 85 +++++++++-------- .../eclipse/tycho/test/util/HttpServer.java | 94 ++++++++++--------- 2 files changed, 97 insertions(+), 82 deletions(-) diff --git a/tycho-its/src/test/java/org/eclipse/tycho/test/tycho2938/ContentJarTest.java b/tycho-its/src/test/java/org/eclipse/tycho/test/tycho2938/ContentJarTest.java index ac28b6d3d7..2d3f93b2b1 100644 --- a/tycho-its/src/test/java/org/eclipse/tycho/test/tycho2938/ContentJarTest.java +++ b/tycho-its/src/test/java/org/eclipse/tycho/test/tycho2938/ContentJarTest.java @@ -19,7 +19,9 @@ import java.nio.file.Path; import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; -import java.util.Arrays; +import java.util.List; + +import javax.xml.parsers.ParserConfigurationException; import org.apache.commons.io.FileUtils; import org.apache.maven.it.VerificationException; @@ -34,17 +36,25 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.xml.sax.SAXException; public class ContentJarTest extends AbstractTychoIntegrationTest { private HttpServer server; + private static final String TARGET_FEATURE_PATH = "/features/issue_2938_reproducer_1.0.0.202310211419.jar"; @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder(); + private Verifier verifier; + private String mainRepoUrl; @Before public void startServer() throws Exception { server = HttpServer.startServer(); File originalResource = ResourceUtil.resolveTestResource("repositories/content_jar"); FileUtils.copyDirectory(originalResource, temporaryFolder.getRoot()); + verifier = getVerifier("target.content_jar", false); + verifier.deleteArtifacts("p2.org.eclipse.update.feature", "issue_2938_reproducer", "1.0.0.202310211419"); + File repositoryRoot = temporaryFolder.getRoot(); + this.mainRepoUrl = server.addServer("repoA", repositoryRoot); } @After @@ -54,49 +64,29 @@ public void stopServer() throws Exception { } } - private void mangleFileNames(Path repositoryRoot) throws IOException { - Files.walkFileTree(repositoryRoot, new SimpleFileVisitor() { - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - Files.move(file, file.getParent().resolve(file.getFileName() + "_invalid")); - return super.visitFile(file, attrs); - } - }); - } - @Test public void noRedirect() throws Exception { - File repositoryRoot = temporaryFolder.getRoot(); - String repoUrl = server.addServer("repoA", repositoryRoot); - Verifier verifier = getVerifier("target.content_jar", false); - File platformFile = new File(verifier.getBasedir(), "targetplatform.target"); - TargetDefinitionUtil.setRepositoryURLs(platformFile, "repoA", repoUrl); - verifier.executeGoals(Arrays.asList("package")); + configureRepositoryInTargetDefinition(mainRepoUrl); + verifier.executeGoal("package"); + assertVisited(TARGET_FEATURE_PATH); verifier.verifyErrorFreeLog(); } @Test public void redirectKeepFilename() throws Exception { - File repositoryRoot = temporaryFolder.getRoot(); - String repoUrl = server.addServer("repoA", repositoryRoot); - String redirectedUrl = server.addRedirect("repoB", originalPath -> repoUrl + originalPath); - Verifier verifier = getVerifier("target.content_jar", false); - File platformFile = new File(verifier.getBasedir(), "targetplatform.target"); - TargetDefinitionUtil.setRepositoryURLs(platformFile, "repoA", redirectedUrl); - verifier.executeGoals(Arrays.asList("package")); + String redirectedUrl = server.addRedirect("repoB", originalPath -> mainRepoUrl + originalPath); + configureRepositoryInTargetDefinition(redirectedUrl); + verifier.executeGoal("package"); + assertVisited(TARGET_FEATURE_PATH); verifier.verifyErrorFreeLog(); } @Test public void redirectToBadLocation() throws Exception { - File repositoryRoot = temporaryFolder.getRoot(); - String repoUrl = server.addServer("repoA", repositoryRoot); - String redirectedUrl = server.addRedirect("repoB", originalPath -> repoUrl + originalPath + "_invalid"); - Verifier verifier = getVerifier("target.content_jar", false); - File platformFile = new File(verifier.getBasedir(), "targetplatform.target"); - TargetDefinitionUtil.setRepositoryURLs(platformFile, "repoA", redirectedUrl); + String redirectedUrl = server.addRedirect("repoB", originalPath -> mainRepoUrl + originalPath + "_invalid"); + configureRepositoryInTargetDefinition(redirectedUrl); Assert.assertThrows(VerificationException.class, () -> verifier.executeGoal("package")); - + assertVisited("/content.jar_invalid"); verifier.verifyTextInLog("No repository found at " + redirectedUrl); } @@ -104,17 +94,38 @@ public void redirectToBadLocation() throws Exception { public void redirectToMangledLocations() throws Exception { File repositoryRoot = temporaryFolder.getRoot(); mangleFileNames(repositoryRoot.toPath()); - String mangledRepoUrl = server.addServer("repoA", repositoryRoot); // https://github.com/eclipse-tycho/tycho/issues/2938 // Redirect may change extension. - String originaRepoUrl = server.addRedirect("repoB", originalPath -> mangledRepoUrl + originalPath + "_invalid"); + String redirectedUrl = server.addRedirect("repoB", originalPath -> mainRepoUrl + originalPath + "_invalid"); - Verifier verifier = getVerifier("target.content_jar", false); - File platformFile = new File(verifier.getBasedir(), "targetplatform.target"); - TargetDefinitionUtil.setRepositoryURLs(platformFile, "repoA", originaRepoUrl); - verifier.executeGoals(Arrays.asList("package")); + configureRepositoryInTargetDefinition(redirectedUrl); + verifier.executeGoal("package"); + assertVisited("/content.jar_invalid"); + assertVisited(TARGET_FEATURE_PATH + "_invalid"); verifier.verifyErrorFreeLog(); } + private void assertVisited(String path) { + List accessedUrls = server.getAccessedUrls("repoA"); + Assert.assertTrue(String.format("Path %s should be visited, %s were visited instead", path, accessedUrls), + accessedUrls.contains(path)); + } + + private void mangleFileNames(Path repositoryRoot) throws IOException { + Files.walkFileTree(repositoryRoot, new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + Files.move(file, file.getParent().resolve(file.getFileName() + "_invalid")); + return super.visitFile(file, attrs); + } + }); + } + + private void configureRepositoryInTargetDefinition(String url) + throws IOException, ParserConfigurationException, SAXException { + File platformFile = new File(verifier.getBasedir(), "targetplatform.target"); + TargetDefinitionUtil.setRepositoryURLs(platformFile, "repoA", url); + } + } 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 191606fc5c..a67fedffa3 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 @@ -14,14 +14,18 @@ import java.io.File; import java.io.IOException; -import java.util.ArrayList; -import java.util.HashMap; +import java.util.EnumSet; import java.util.List; -import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.function.Function; +import javax.servlet.DispatcherType; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -37,42 +41,37 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.ContextHandlerCollection; import org.eclipse.jetty.servlet.DefaultServlet; +import org.eclipse.jetty.servlet.FilterHolder; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jetty.util.MultiMap; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.security.Constraint; import org.eclipse.jetty.util.security.Password; public class HttpServer { - private static class MonitoringServlet extends DefaultServlet { - private List accessedURIs = new ArrayList<>(); + private class Monitoring implements Filter { @Override - public String getInitParameter(String name) { - // no directory listing allowed - if ("dirAllowed".equals(name)) { - return "false"; - } else { - return super.getInitParameter(name); - } - } - - public List getAccessedURIs() { - return accessedURIs; + public void init(FilterConfig filterConfig) throws ServletException { } @Override - protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - accessedURIs.add(((Request) request).getHttpURI().toString()); - super.doGet(request, response); + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + Request httprequest = Request.getBaseRequest(request); + String context = httprequest.getContextPath(); + assert context.startsWith("/"); + context = context.substring(1); + synchronized (contextName2accessedUrls) { + contextName2accessedUrls.add(context, httprequest.getServletPath()); + } + chain.doFilter(request, response); } @Override - protected void doPost(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - accessedURIs.add(((Request) request).getHttpURI().toString()); - super.doPost(request, response); + public void destroy() { + } } @@ -97,7 +96,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se private final int port; - private final Map contextName2servletsMap = new HashMap<>(); + private final MultiMap contextName2accessedUrls = new MultiMap<>(); private ContextHandlerCollection contexts; @@ -170,32 +169,26 @@ public void stop() throws Exception { public String addServer(String contextName, final File content) { ServletContextHandler context = new ServletContextHandler(contexts, URIUtil.SLASH + contextName); + context.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false"); context.setResourceBase(content.getAbsolutePath()); - - addMonitoring(contextName, context); - try { - context.start(); - } catch (Exception e) { - throw new RuntimeException(e); - } + context.addServlet(DefaultServlet.class, URIUtil.SLASH); + registerContext(context); return getUrl(contextName); } - private void addMonitoring(String contextName, ServletContextHandler context) { - MonitoringServlet monitoringServlet = new MonitoringServlet(); - contextName2servletsMap.put(contextName, monitoringServlet); - context.addServlet(new ServletHolder(monitoringServlet), URIUtil.SLASH); - contexts.addHandler(context); - } - + /** + * + * @param contextName - a path prefix to handle + * @param relativeUrlToNewUrl - redirecting function. Takes a path within + * context starting with slash and return an new + * absolute URL to redirect to (Location header + * value). + * @return an URL prefix to redirect from + */ public String addRedirect(String contextName, Function relativeUrlToNewUrl) { ServletContextHandler context = new ServletContextHandler(contexts, URIUtil.SLASH + contextName); context.addServlet(new ServletHolder(new RedirectServlet(relativeUrlToNewUrl)), URIUtil.SLASH); - try { - context.start(); - } catch (Exception e) { - throw new RuntimeException(e); - } + registerContext(context); return getUrl(contextName); } @@ -205,7 +198,18 @@ public String getUrl(String contextName) { } public List getAccessedUrls(String contextName) { - return contextName2servletsMap.get(contextName).getAccessedURIs(); + synchronized (contextName2accessedUrls) { + return List.copyOf(contextName2accessedUrls.get(contextName)); + } } + private void registerContext(ServletContextHandler context) { + context.addFilter(new FilterHolder(new Monitoring()), "*", EnumSet.of(DispatcherType.REQUEST)); + contexts.addHandler(context); + try { + context.start(); + } catch (Exception e) { + throw new RuntimeException(e); + } + } }