Copilot commented on code in PR #8697:
URL: https://github.com/apache/hadoop/pull/8697#discussion_r3826569560
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/WebAppProxyServlet.java:
##########
@@ -310,7 +310,10 @@ private void proxyLink(final HttpServletRequest req,
base.setHeader(name, value);
}
}
-
+ // Tell the AM/history server to close the connection after the response
+ // so that the proxied connection is not left in CLOSE_WAIT state on the
+ // proxy side (SOHU-HADOOP-11).
+ base.setHeader("Connection", "close");
Review Comment:
The new comment references "SOHU-HADOOP-11", which doesn’t match the public
tracking ID used elsewhere in this PR (YARN-11845). Please use the YARN JIRA ID
here to keep the codebase references consistent.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServlet.java:
##########
@@ -481,6 +503,75 @@ void testWebAppProxyPassThroughHeaders() throws Exception {
}
}
+ /**
+ * Test that the proxy tells the proxied server (AM / history server) to
+ * close the connection after each response. The proxy builds a new
+ * HttpClient per request, so if the backend connection is kept alive the
+ * socket is left in CLOSE_WAIT state on the proxy host until GC reclaims
+ * it. Setting the 'Connection: close' request header makes the backend
+ * close the connection as soon as the response is sent (SOHU-HADOOP-11).
+ */
+ @Test
+ @Timeout(5000)
+ void testWebAppProxyConnectionCloseHeader() throws Exception {
+ Configuration configuration = new Configuration();
+ configuration.set(YarnConfiguration.PROXY_ADDRESS, "localhost:9093");
+ configuration.setInt("hadoop.http.max.threads", 10);
+ WebAppProxyServerForTest proxy = new WebAppProxyServerForTest();
+ proxy.init(configuration);
+ proxy.start();
+
+ int proxyPort = proxy.proxy.proxyServer.getConnectorAddress(0).getPort();
+ proxy.proxy.appReportFetcher.answer = 0;
+
+ try {
+ // GET: the proxied request must carry 'Connection: close'
+ URL url = new URL("http://localhost:"; + proxyPort
+ + "/proxy/application_00_0");
+ HttpURLConnection proxyConn = (HttpURLConnection) url.openConnection();
+ proxyConn.setRequestProperty("Cookie",
+ "checked_application_0_0000=true");
+ proxyConn.connect();
+ assertEquals(HttpURLConnection.HTTP_OK, proxyConn.getResponseCode());
+ assertNotNull(proxiedConnectionHeader,
+ "The proxied server did not receive a Connection header at all");
+ assertEquals("close", proxiedConnectionHeader.trim().toLowerCase(),
+ "The proxy must send 'Connection: close' to the proxied server "
+ + "so the backend closes the connection instead of leaving it "
+ + "in CLOSE_WAIT state (SOHU-HADOOP-11)");
+
+ // even if the incoming client request asked for keep-alive, the proxy
+ // must still ask the backend to close the connection
+ proxyConn = (HttpURLConnection) url.openConnection();
+ proxyConn.setRequestProperty("Cookie",
+ "checked_application_0_0000=true");
+ proxyConn.setRequestProperty("Connection", "keep-alive");
+ proxyConn.connect();
+ assertEquals(HttpURLConnection.HTTP_OK, proxyConn.getResponseCode());
+ assertEquals("close", proxiedConnectionHeader.trim().toLowerCase(),
+ "The proxy must override a client 'Connection: keep-alive' "
+ + "request header with 'close' (SOHU-HADOOP-11)");
Review Comment:
This second request reuses the previously-populated
`proxiedConnectionHeader`. If the second proxied request fails (or hits a
different path) the assertion could still pass using the value from the first
request. Reset the static capture and assert it was set for each request to
avoid false positives.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServlet.java:
##########
@@ -481,6 +503,75 @@ void testWebAppProxyPassThroughHeaders() throws Exception {
}
}
+ /**
+ * Test that the proxy tells the proxied server (AM / history server) to
+ * close the connection after each response. The proxy builds a new
+ * HttpClient per request, so if the backend connection is kept alive the
+ * socket is left in CLOSE_WAIT state on the proxy host until GC reclaims
+ * it. Setting the 'Connection: close' request header makes the backend
+ * close the connection as soon as the response is sent (SOHU-HADOOP-11).
+ */
Review Comment:
This test (and the new static field comment above) references
"SOHU-HADOOP-11" in multiple places, but the public issue tracked by this PR is
YARN-11845. Please replace these internal identifiers with the YARN JIRA ID for
consistency and long-term maintainability.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServlet.java:
##########
@@ -97,6 +97,10 @@ public class TestWebAppProxyServlet {
private static int numberOfHeaders = 0;
private static final String UNKNOWN_HEADER = "Unknown-Header";
private static boolean hasUnknownHeader = false;
+ // Value of the Connection header received by the proxied server (the
+ // embedded TestServlet backend). Used to verify the proxy sends
+ // 'Connection: close' (SOHU-HADOOP-11).
+ private static String proxiedConnectionHeader = null;
Review Comment:
`proxiedConnectionHeader` is written by the embedded servlet thread and read
by the test thread. Without `volatile` (or other synchronization), this can be
a data-race and lead to intermittent visibility issues/flaky assertions on some
JVMs.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServlet.java:
##########
@@ -481,6 +503,75 @@ void testWebAppProxyPassThroughHeaders() throws Exception {
}
}
+ /**
+ * Test that the proxy tells the proxied server (AM / history server) to
+ * close the connection after each response. The proxy builds a new
+ * HttpClient per request, so if the backend connection is kept alive the
+ * socket is left in CLOSE_WAIT state on the proxy host until GC reclaims
+ * it. Setting the 'Connection: close' request header makes the backend
+ * close the connection as soon as the response is sent (SOHU-HADOOP-11).
+ */
+ @Test
+ @Timeout(5000)
+ void testWebAppProxyConnectionCloseHeader() throws Exception {
+ Configuration configuration = new Configuration();
+ configuration.set(YarnConfiguration.PROXY_ADDRESS, "localhost:9093");
+ configuration.setInt("hadoop.http.max.threads", 10);
+ WebAppProxyServerForTest proxy = new WebAppProxyServerForTest();
+ proxy.init(configuration);
+ proxy.start();
+
+ int proxyPort = proxy.proxy.proxyServer.getConnectorAddress(0).getPort();
+ proxy.proxy.appReportFetcher.answer = 0;
+
+ try {
+ // GET: the proxied request must carry 'Connection: close'
+ URL url = new URL("http://localhost:"; + proxyPort
+ + "/proxy/application_00_0");
+ HttpURLConnection proxyConn = (HttpURLConnection) url.openConnection();
+ proxyConn.setRequestProperty("Cookie",
+ "checked_application_0_0000=true");
+ proxyConn.connect();
+ assertEquals(HttpURLConnection.HTTP_OK, proxyConn.getResponseCode());
+ assertNotNull(proxiedConnectionHeader,
+ "The proxied server did not receive a Connection header at all");
+ assertEquals("close", proxiedConnectionHeader.trim().toLowerCase(),
Review Comment:
Using `String#toLowerCase()` without an explicit locale can cause
locale-sensitive behavior (e.g., Turkish locale). Since this test is doing a
protocol/value comparison, use Hadoop’s `StringUtils.toLowerCase()`
(Locale.ENGLISH) instead.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServlet.java:
##########
@@ -481,6 +503,75 @@ void testWebAppProxyPassThroughHeaders() throws Exception {
}
}
+ /**
+ * Test that the proxy tells the proxied server (AM / history server) to
+ * close the connection after each response. The proxy builds a new
+ * HttpClient per request, so if the backend connection is kept alive the
+ * socket is left in CLOSE_WAIT state on the proxy host until GC reclaims
+ * it. Setting the 'Connection: close' request header makes the backend
+ * close the connection as soon as the response is sent (SOHU-HADOOP-11).
+ */
+ @Test
+ @Timeout(5000)
+ void testWebAppProxyConnectionCloseHeader() throws Exception {
+ Configuration configuration = new Configuration();
+ configuration.set(YarnConfiguration.PROXY_ADDRESS, "localhost:9093");
+ configuration.setInt("hadoop.http.max.threads", 10);
+ WebAppProxyServerForTest proxy = new WebAppProxyServerForTest();
+ proxy.init(configuration);
+ proxy.start();
+
+ int proxyPort = proxy.proxy.proxyServer.getConnectorAddress(0).getPort();
+ proxy.proxy.appReportFetcher.answer = 0;
+
+ try {
+ // GET: the proxied request must carry 'Connection: close'
+ URL url = new URL("http://localhost:"; + proxyPort
+ + "/proxy/application_00_0");
+ HttpURLConnection proxyConn = (HttpURLConnection) url.openConnection();
+ proxyConn.setRequestProperty("Cookie",
+ "checked_application_0_0000=true");
+ proxyConn.connect();
+ assertEquals(HttpURLConnection.HTTP_OK, proxyConn.getResponseCode());
+ assertNotNull(proxiedConnectionHeader,
+ "The proxied server did not receive a Connection header at all");
+ assertEquals("close", proxiedConnectionHeader.trim().toLowerCase(),
+ "The proxy must send 'Connection: close' to the proxied server "
+ + "so the backend closes the connection instead of leaving it "
+ + "in CLOSE_WAIT state (SOHU-HADOOP-11)");
+
+ // even if the incoming client request asked for keep-alive, the proxy
+ // must still ask the backend to close the connection
+ proxyConn = (HttpURLConnection) url.openConnection();
+ proxyConn.setRequestProperty("Cookie",
+ "checked_application_0_0000=true");
+ proxyConn.setRequestProperty("Connection", "keep-alive");
+ proxyConn.connect();
+ assertEquals(HttpURLConnection.HTTP_OK, proxyConn.getResponseCode());
+ assertEquals("close", proxiedConnectionHeader.trim().toLowerCase(),
+ "The proxy must override a client 'Connection: keep-alive' "
+ + "request header with 'close' (SOHU-HADOOP-11)");
+
+ // PUT: the header must be set on PUT requests as well
+ proxyConn = (HttpURLConnection) url.openConnection();
+ proxyConn.setRequestMethod("PUT");
+ proxyConn.setDoOutput(true);
+ proxyConn.setRequestProperty("Cookie",
+ "checked_application_0_0000=true");
+ proxyConn.connect();
+ byte[] body = "SOHU-HADOOP-11".getBytes(StandardCharsets.UTF_8);
+ try (OutputStream os = proxyConn.getOutputStream()) {
+ os.write(body);
+ }
+ assertEquals(HttpURLConnection.HTTP_OK, proxyConn.getResponseCode());
+ assertEquals("close", proxiedConnectionHeader.trim().toLowerCase(),
+ "The proxy must send 'Connection: close' on PUT requests too "
+ + "(SOHU-HADOOP-11)");
Review Comment:
Same as the previous case: reset and re-assert `proxiedConnectionHeader` for
the PUT request so the test can’t pass with a stale value from the earlier GET.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
