[GitHub] [flink] 1u0 commented on a change in pull request #9767: [FLINK-14120][API/Datastream] Fix testImmediateShutdown

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

1u0 commented on a change in pull request #9767: [FLINK-14120][API/Datastream] Fix testImmediateShutdown
URL: https://github.com/apache/flink/pull/9767#discussion_r329113036
 
 

 ##########
 File path: flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/tasks/SystemProcessingTimeServiceTest.java
 ##########
 @@ -154,27 +152,25 @@ public void testImmediateShutdown() throws Exception {
 			// the task should trigger immediately and should block until terminated with interruption
 			timer.registerTimer(System.currentTimeMillis(), timestamp -> {
 				synchronized (lock) {
-					latch.trigger();
-					Thread.sleep(100000000);
+					try {
+						latch.trigger();
+						Thread.sleep(100000000);
+						fail("should be interrupted");
+					} catch (InterruptedException ignored) {
+					}
 				}
 			});
 
 			latch.await();
 			timer.shutdownService();
+
+			// can only enter this scope after the task is interrupted
 			synchronized (lock) {
 				assertTrue(timer.isTerminated());
-
-				// The shutdownService() may not necessary wait for active tasks to finish properly.
-				// From the ScheduledThreadPoolExecutor Java docs:
-				// 	There are no guarantees beyond best-effort attempts to stop processing actively executing tasks.
-				// 	This implementation cancels tasks via {@link Thread#interrupt}, so any task that
-				// 	fails to respond to interrupts may never terminate.
-				assertThat(errorRef.get(), is(anyOf(nullValue(), instanceOf(InterruptedException.class))));
 
 Review comment:
   @a-siuniaev, thanks for explanation and the fix.
   
   Just for information, the test was checking that the first timer callback may report exception (via callback, although with wrong synchronization). But the second and third timer callbacks won't use exception reporting mechanism, but rather throw at timer registration call. This is basically the reason why the assert that checks error was pulled up, near the first timer and for other timers, the similar check was verifying that there is no reported error.
   
   In some tests, the error was `null`  and my assumption was that thread could have "interrupted" by force (without allowing to call any follow up code. But looks like it was rather a data race).
   An alternative fix could be to move setting `errorRef` under the same critical section.

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


With regards,
Apache Git Services