[GitHub] [flink] scudellari commented on a change in pull request #17227: [FLINK-24156][runtime/network] Catch erroneous SocketTimeoutExceptions and retry
scudellari commented on a change in pull request #17227:
URL: https://github.com/apache/flink/pull/17227#discussion_r706110378
##
File path: flink-core/src/test/java/org/apache/flink/util/NetUtilsTest.java
##
@@ -61,25 +56,45 @@ public void testParseHostPortAddress() {
@Test
public void testAcceptWithoutTimeout() throws IOException {
// Validates that acceptWithoutTimeout suppresses all
SocketTimeoutExceptions
-ServerSocket serverSocket = mock(ServerSocket.class);
-when(serverSocket.accept())
-.thenAnswer(
-new Answer() {
-private int count = 0;
-
-@Override
-public Socket answer(InvocationOnMock
invocationOnMock)
-throws Throwable {
-if (count < 2) {
-count++;
-throw new SocketTimeoutException();
-}
-
-return new Socket();
-}
-});
-
-assertNotNull(NetUtils.acceptWithoutTimeout(serverSocket));
+Socket expected = new Socket();
+ServerSocket serverSocket =
+new ServerSocket() {
+private int count = 0;
+
+@Override
+public Socket accept() throws IOException {
+if (count < 2) {
+count++;
+throw new SocketTimeoutException();
+}
+
+return expected;
+}
+};
+
+assertEquals(expected, NetUtils.acceptWithoutTimeout(serverSocket));
+
+// Validates timeout option precondition
+serverSocket =
+new ServerSocket() {
+@Override
+public Socket accept() throws IOException {
+return expected;
+}
+};
+
+// non-zero timeout (throw exception)
+serverSocket.setSoTimeout(5);
+try {
+assertEquals(expected,
NetUtils.acceptWithoutTimeout(serverSocket));
+fail("Expected IllegalArgumentException due to timeout is set to
non-zero value");
+} catch (IllegalArgumentException e) {
+// Pass
+}
+
+// zero timeout (don't throw exception)
+serverSocket.setSoTimeout(0);
+assertEquals(expected, NetUtils.acceptWithoutTimeout(serverSocket));
Review comment:
I added this to cover the case where we explicitly set the timeout to
zero. Initially we are simply using the default value (which is also zero).
Granted, this is likely overkill.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] scudellari commented on a change in pull request #17227: [FLINK-24156][runtime/network] Catch erroneous SocketTimeoutExceptions and retry
scudellari commented on a change in pull request #17227:
URL: https://github.com/apache/flink/pull/17227#discussion_r705698738
##
File path: flink-core/src/main/java/org/apache/flink/util/NetUtils.java
##
@@ -116,6 +119,39 @@ private static URL validateHostPortString(String hostPort)
{
}
}
+/**
+ * Calls {@link ServerSocket#accept()} on the provided server socket,
suppressing any thrown
+ * {@link SocketTimeoutException}s. This is a workaround for the
underlying JDK-8237858 bug in
+ * JDK 11 that can cause errant SocketTimeoutExceptions to be thrown at
unexpected times.
+ *
+ * This method expects the provided ServerSocket has no timeout set
(SO_TIMEOUT of 0),
+ * indicating an infinite timeout. It will suppress all
SocketTimeoutExceptions, even if a
+ * ServerSocket with a non-zero timeout is passed in.
+ *
+ * @param serverSocket a ServerSocket with {@link SocketOptions#SO_TIMEOUT
SO_TIMEOUT} set to 0;
+ * if SO_TIMEOUT is greater than 0, then this method will suppress
SocketTimeoutException;
Review comment:
Sounds good
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] scudellari commented on a change in pull request #17227: [FLINK-24156][runtime/network] Catch erroneous SocketTimeoutExceptions and retry
scudellari commented on a change in pull request #17227:
URL: https://github.com/apache/flink/pull/17227#discussion_r705677409
##
File path: flink-core/src/test/java/org/apache/flink/util/NetUtilsTest.java
##
@@ -49,6 +58,30 @@ public void testParseHostPortAddress() {
assertEquals(socketAddress,
NetUtils.parseHostPortAddress("foo.com:8080"));
}
+@Test
+public void testAcceptWithoutTimeout() throws IOException {
+// Validates that acceptWithoutTimeout suppresses all
SocketTimeoutExceptions
+ServerSocket serverSocket = mock(ServerSocket.class);
Review comment:
Good point, will do. Out of curiosity, what is the motivation for
avoiding mockito? (Apologies if this is well documented already)
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] scudellari commented on a change in pull request #17227: [FLINK-24156][runtime/network] Catch erroneous SocketTimeoutExceptions and retry
scudellari commented on a change in pull request #17227:
URL: https://github.com/apache/flink/pull/17227#discussion_r705676769
##
File path: flink-core/src/main/java/org/apache/flink/util/NetUtils.java
##
@@ -116,6 +119,39 @@ private static URL validateHostPortString(String hostPort)
{
}
}
+/**
+ * Calls {@link ServerSocket#accept()} on the provided server socket,
suppressing any thrown
+ * {@link SocketTimeoutException}s. This is a workaround for the
underlying JDK-8237858 bug in
+ * JDK 11 that can cause errant SocketTimeoutExceptions to be thrown at
unexpected times.
+ *
+ * This method expects the provided ServerSocket has no timeout set
(SO_TIMEOUT of 0),
+ * indicating an infinite timeout. It will suppress all
SocketTimeoutExceptions, even if a
+ * ServerSocket with a non-zero timeout is passed in.
+ *
+ * @param serverSocket a ServerSocket with {@link SocketOptions#SO_TIMEOUT
SO_TIMEOUT} set to 0;
+ * if SO_TIMEOUT is greater than 0, then this method will suppress
SocketTimeoutException;
Review comment:
We could. I started down this path, but I was not sure what the
performance implications of calling `getSoTimeout` would be. The fact that it
throws IOExceptions gave me pause. Given that there is only a single case where
a socket is setting a timeout (in a test none the less) I felt it was safer to
avoid the call altogether.
Granted, this landscape could change over time and this certainly would be a
confusing bug. (suppressing timeouts unexpectedly) I named the method in a way
that I hoped it would minimize the chances of this sort of thing.
I'm happy to go either way.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
