[GitHub] [flink] pnowojski commented on a change in pull request #12261: [FLINK-17823][network] Resolve the race condition while releasing RemoteInputChannel

[GitBox <gi...@apache.org>]

pnowojski commented on a change in pull request #12261:
URL: https://github.com/apache/flink/pull/12261#discussion_r428179383



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/RemoteInputChannel.java
##########
@@ -181,6 +181,14 @@ void retriggerSubpartitionRequest(int subpartitionIndex) throws IOException {
 			moreAvailable = !receivedBuffers.isEmpty();
 		}
 
+		if (next == null) {

Review comment:
       What do you mean @zhijiangW ? At first glance I would agree with @Jiayi-Liao, that it shouldn't happen after your fix in this commit in this class below.

##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/consumer/RemoteInputChannelTest.java
##########
@@ -1010,6 +1011,56 @@ public void testConcurrentRecycleAndRelease2() throws Exception {
 		}
 	}
 
+	@Test
+	public void testConcurrentGetNextBufferAndRelease() throws Exception {

Review comment:
       I think we could have tested this bug without concurrency/multi threading by just calling `releaseAllResources` before `getNextBuffer`. Test would be much easier to understand and debug and this would be worth a bit worse testing coverage - especially that we still have ITCases, and in the long run we are planning/hoping to make releasing resources go through mailbox.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org