From f1982155ad2d9c598990079987e812900fea06e0 Mon Sep 17 00:00:00 2001 From: Raghav Aggarwal Date: Sun, 19 Jul 2026 16:31:14 +0530 Subject: [PATCH] TEZ-4743: Refactor ProfileServlet and ProfileOutputServlet to use standard response.sendError API - Replaced manual error writing with the standard, built-in response.sendError(...) API in both servlets - Removed the obsolete writeMessage() and PrintStream logic - If the `?file=` parameter is missing, the servlet now correctly returns a 400 Bad Request instead of incorrectly returning a 200 OK success code --- .../tez/common/web/ProfileOutputServlet.java | 26 ++++------ .../apache/tez/common/web/ProfileServlet.java | 48 +++++++------------ .../test/java/org/apache/tez/test/TestAM.java | 2 +- 3 files changed, 28 insertions(+), 48 deletions(-) diff --git a/tez-common/src/main/java/org/apache/tez/common/web/ProfileOutputServlet.java b/tez-common/src/main/java/org/apache/tez/common/web/ProfileOutputServlet.java index 7d36339540..e92af49267 100644 --- a/tez-common/src/main/java/org/apache/tez/common/web/ProfileOutputServlet.java +++ b/tez-common/src/main/java/org/apache/tez/common/web/ProfileOutputServlet.java @@ -20,15 +20,14 @@ import java.io.File; import java.io.IOException; -import java.io.PrintWriter; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.apache.hadoop.http.HtmlQuoting; import org.apache.hadoop.http.HttpServer2; import org.apache.hadoop.yarn.webapp.MimeType; @@ -40,34 +39,34 @@ public class ProfileOutputServlet extends DefaultServlet { public static final String FILE_QUERY_PARAM = "file"; - public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException { if (!HttpServer2.isInstrumentationAccessAllowed(this.getServletContext(), request, response)) { - response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); - writeMessage(response, ProfileServlet.ACCESS_DENIED_MESSAGE); + response.sendError(HttpServletResponse.SC_UNAUTHORIZED, ProfileServlet.ACCESS_DENIED_MESSAGE); return; } String queriedFile = request.getParameter(FILE_QUERY_PARAM); if (queriedFile == null) { - writeMessage(response, "Run the profiler to be able to receive its output"); + response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Run the profiler to be able to receive its output"); return; } Path outputDir = Paths.get(ProfileServlet.OUTPUT_DIR).toAbsolutePath().normalize(); Path requestedPath = outputDir.resolve(queriedFile).normalize(); if (!requestedPath.startsWith(outputDir)) { - response.setStatus(HttpServletResponse.SC_FORBIDDEN); - writeMessage(response, "Access denied: Invalid Path"); + response.sendError(HttpServletResponse.SC_FORBIDDEN, "Access denied: Invalid Path"); return; } File outputFile = requestedPath.toFile(); if (!outputFile.exists()) { - writeMessage(response, "Requested file does not exist: " + queriedFile); + response.sendError(HttpServletResponse.SC_NOT_FOUND, + "Requested file does not exist: " + HtmlQuoting.quoteHtmlChars(queriedFile)); return; } if (outputFile.length() < 100) { response.setIntHeader("Refresh", 2); - writeMessage(response, "This page auto-refreshes every 2 seconds until output file is ready..."); + response.setContentType(MimeType.TEXT); + response.getWriter().println("This page auto-refreshes every 2 seconds until output file is ready..."); return; } response.setContentType(MimeType.HTML); @@ -75,11 +74,4 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro response.getOutputStream().flush(); response.getOutputStream().close(); } - - private void writeMessage(HttpServletResponse response, String message) throws IOException { - response.setContentType(MimeType.TEXT); - PrintWriter out = response.getWriter(); - out.println(message); - out.close(); - } } diff --git a/tez-common/src/main/java/org/apache/tez/common/web/ProfileServlet.java b/tez-common/src/main/java/org/apache/tez/common/web/ProfileServlet.java index ef3d992a24..feab37c9ff 100644 --- a/tez-common/src/main/java/org/apache/tez/common/web/ProfileServlet.java +++ b/tez-common/src/main/java/org/apache/tez/common/web/ProfileServlet.java @@ -20,7 +20,7 @@ import java.io.File; import java.io.IOException; -import java.io.PrintStream; +import java.io.PrintWriter; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -31,7 +31,6 @@ import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; -import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -161,23 +160,15 @@ public ProfileServlet() { LOG.info("Servlet process PID: {} asyncProfilerHome: {}", pid, asyncProfilerHome); } - public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { - response.setContentType("text/plain; charset=UTF-8"); - PrintStream out = new PrintStream(response.getOutputStream(), false, "UTF-8"); + public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException { if (!HttpServer2.isInstrumentationAccessAllowed(this.getServletContext(), request, response)) { - response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); - setResponseHeader(response); - out.println(ACCESS_DENIED_MESSAGE); - out.close(); + response.sendError(HttpServletResponse.SC_UNAUTHORIZED, ACCESS_DENIED_MESSAGE); return; } // make sure async profiler home is set if (asyncProfilerHome == null || asyncProfilerHome.trim().isEmpty()) { - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - setResponseHeader(response); - out.println("ASYNC_PROFILER_HOME env is not set"); - out.close(); + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "ASYNC_PROFILER_HOME env is not set"); return; } @@ -185,10 +176,8 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro pid = getInteger(request, "pid", pid); // if pid is not specified in query param and if current process pid cannot be determined if (pid == null) { - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - setResponseHeader(response); - out.println("'pid' query parameter unspecified or unable to determine PID of current process."); - out.close(); + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + "'pid' query parameter unspecified or unable to determine PID of current process."); return; } @@ -278,41 +267,40 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro response.setHeader("Refresh", (duration + refreshDelay) + "; URL=" + relativeUrl + '?' + ProfileOutputServlet.FILE_QUERY_PARAM + '=' + outputFile.getName()); - out.println("Profiled PID: " + pid); - out.println("Started [" + event.getInternalName() - + "] profiling. This page will automatically redirect to " - + relativeUrl + " after " + duration + " seconds.\n\ncommand:\n" + Joiner.on(" ").join(cmd)); - out.flush(); + PrintWriter writer = response.getWriter(); + writer.println("Profiled PID: " + pid); + writer.println( + "Started [" + event.getInternalName() + "] profiling. This page will automatically redirect to " + + relativeUrl + " after " + duration + " seconds.\n\ncommand:\n" + Joiner.on(" ").join(cmd)); + writer.flush(); } finally { profilerLock.unlock(); } } else { - setResponseHeader(response); - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - out.println("Unable to acquire lock. Another instance of profiler might be running."); LOG.warn("Unable to acquire lock in {} seconds. Another instance of profiler might be running.", lockTimeoutSecs); + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + "Unable to acquire lock. Another instance of profiler might be running."); } } catch (InterruptedException e) { LOG.warn("Interrupted while acquiring profile lock.", e); response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } } else { - setResponseHeader(response); - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - out.println("Another instance of profiler is already running."); + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + "Another instance of profiler is already running."); } - out.close(); } /** * Get the path of the profiler script to be executed. * Before async-profiler 3.0, the script was named profiler.sh, and after 3.0 it's bin/asprof + * * @return */ private String getProfilerScriptPath() { Path defaultPath = Paths.get(asyncProfilerHome + "/bin/asprof"); - return Files.exists(defaultPath)? defaultPath.toString() : asyncProfilerHome + "/profiler.sh"; + return Files.exists(defaultPath) ? defaultPath.toString() : asyncProfilerHome + "/profiler.sh"; } private Integer getInteger(final HttpServletRequest req, final String param, final Integer defaultValue) { diff --git a/tez-tests/src/test/java/org/apache/tez/test/TestAM.java b/tez-tests/src/test/java/org/apache/tez/test/TestAM.java index b07f174b68..a6b0dcc4f1 100644 --- a/tez-tests/src/test/java/org/apache/tez/test/TestAM.java +++ b/tez-tests/src/test/java/org/apache/tez/test/TestAM.java @@ -139,7 +139,7 @@ public void testAMWebUIService() throws TezException, IOException, InterruptedEx checkAddress(webUIAddress + "/conf"); checkAddress(webUIAddress + "/stacks"); checkAddress(webUIAddress + "/prof", 202); - checkAddress(webUIAddress + "/prof-output"); + checkAddress(webUIAddress + "/prof-output", HttpServletResponse.SC_BAD_REQUEST); HttpURLConnection connection = (HttpURLConnection) URI.create(webUIAddress + "/prof-output?file=../etc/web").toURL().openConnection();