patricklucas commented on code in PR #22987:
URL: https://github.com/apache/flink/pull/22987#discussion_r1273174325
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java:
##########
@@ -207,6 +210,40 @@ public void testRestClientClosedHandling() throws
Exception {
}
}
+ /**
+ * Tests that the futures returned by {@link RestClient} fail immediately
if the client is
+ * already closed.
+ *
+ * <p>See FLINK-32583
+ */
+ @Test
+ public void testCloseClientBeforeRequest() throws Exception {
Review Comment:
With this additional test, the `isRunning` codepath is now tested, but not
the branch where the listener is unable to be attached and is thus handled in
`notifyResponseFuturesOfShutdown()`. I think there is just a microscopic window
for this to happen, between the calls to `bootstrap.connect()` and
`connectFuture.addListener()`, which are not executed atomically.
The only way that comes to mind to test that would be to inject a latch to
enable calling `close()` between those two calls.
Do you think that's necessary? I could also add a note that that eventuality
was tested manually, with `notifyResponseFuturesOfShutdown()` successfully
resolving the future.
--
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]
