pan3793 commented on code in PR #8472:
URL: https://github.com/apache/hadoop/pull/8472#discussion_r3198712995
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java:
##########
@@ -235,19 +237,45 @@ private static int startRpcServer(boolean
allowInsecurePorts)
"localhost", serverPort, 100000, 1, 2, allowInsecurePorts);
tcpServer = new SimpleTcpServer(serverPort, program, 1);
tcpServer.run();
- break; // Successfully bound a port, break out.
- } catch (InterruptedException | ChannelException e) {
+ return serverPort; // Successfully bound a port.
+ } catch (InterruptedException e) {
if (tcpServer != null) {
tcpServer.shutdown();
}
- if (retries-- > 0) {
- serverPort += rand.nextInt(20); // Port in use? Try another.
- } else {
- throw e; // Out of retries.
+ Thread.currentThread().interrupt();
+ throw e;
+ } catch (Exception e) {
+ // Netty's ChannelFuture#sync may "sneaky-throw" a checked exception
+ // such as java.net.BindException via PlatformDependent#throwException,
+ // which is why catching ChannelException alone is not sufficient.
+ // Only retry on port-in-use style failures; rethrow anything else.
+ if (tcpServer != null) {
+ tcpServer.shutdown();
}
+ if (!isPortInUse(e) || retries-- <= 0) {
+ throw e; // Not a port-in-use error, or out of retries.
+ }
+ // Port in use? Try another. Ensure we never re-pick the same port.
+ serverPort += 1 + rand.nextInt(20);
+ }
+ }
+ }
+
+ /**
+ * Check whether the given exception indicates that the port is already
+ * bound by another process. Handles {@link ChannelException} thrown by
+ * Netty as well as {@link BindException} that may be sneaky-thrown from
+ * {@code ChannelFuture#sync()}.
+ */
+ private static boolean isPortInUse(Throwable t) {
+ Throwable cursor = t;
+ while (cursor != null) {
+ if (cursor instanceof BindException || cursor instanceof
ChannelException) {
+ return true;
Review Comment:
partially adopted. only match `BindException`, but do not check the message
- this is not something in api contract, it might change in future JDK releases.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java:
##########
@@ -22,6 +22,8 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.IOException;
Review Comment:
adopted
--
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]
