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:
[email protected]
With regards,
Apache Git Services
