Make DeterministicRunner#close always block until the closing is done #1255

Spikhalskiy · 2022-06-08T20:17:21Z

What was changed

DeterministicRunner#close now returns always after the closing is fully completed.

Why?

Previous behavior may be causing rejected executions from the cache because the threads were not actually released yet even when the executions are gone from the cache.

How was this tested

Unit test executes two #close in parallel and checking that they return only when the workflow thread is finished which fails on a current master.

Spikhalskiy · 2022-06-09T13:56:29Z

temporal-sdk/src/main/java/io/temporal/internal/sync/DeterministicRunnerImpl.java

    closeRequested = true;
-    if (inRunUntilAllBlocked) {
+    if (


The main important change in this PR is here, everything else are refactorings, cleanups, and comments.

cretz · 2022-06-09T14:58:17Z

temporal-sdk/src/main/java/io/temporal/internal/sync/DeterministicRunnerImpl.java

-    if (inRunUntilAllBlocked) {
+    if (
+    // Do not close immediately if runUntilAllBlocked executes.
+    // closeRequested will tell a control thread to call close() at the end of the event loop.


This comment should remain above the conditional I think since it refers to setting of closeRequested

No, it's for inRunUntilAllBlocked check specifically. Another check in this if is closeStarted, not closeRequested .

I am referring to the moved comment that is talking about closeRequested. It made more since when it was above closeRequested.

Not really, because this comment makes sense only on inRunUntilAllBlocked == true condition. Before it was the only condition, so it made sense there, but not anymore.
I rephrased the comment to make it more clear.

cretz · 2022-06-09T15:03:02Z

temporal-sdk/src/main/java/io/temporal/internal/sync/DeterministicRunnerImpl.java

+    // closeRequested will tell a control thread to call close() at the end of the event loop.
+    inRunUntilAllBlocked
+        // some other thread or caller is already in the process of closing
+        || closeStarted) {


This will only be true if another thread gets to line 360 where this is unlocked right? Otherwise, isn't that what the lock is for? Doesn't the lock prevent multiple threads calling this method?

This will only be true if another thread gets to line 360 where this is unlocked right?

Yes, another thread started a shutdown, set the flag and released the lock, another thread can proceed to here and observe that another shutdown is already in progress.

cretz · 2022-06-09T15:05:11Z

temporal-sdk/src/main/java/io/temporal/internal/sync/DeterministicRunnerImpl.java

      lock.unlock();
+      // We will not perform the closure in this call and should just wait on the future when
+      // another thread responsible for it will close.
+      closeFuture.join();


Any concerns of a hang here? Specifically, if closeFuture.complete is not reached but closeStarted = true is, this code will hang.

No. If the thread started shutdown, but didn't reach the end - it's a bug and something unexpected happened. But to make this code LOOK safer, I will wrap it in try-finally.

cretz · 2022-06-09T15:06:10Z

temporal-sdk/src/main/java/io/temporal/internal/sync/DeterministicRunnerImpl.java

+    // closeRequested will tell a control thread to call close() at the end of the event loop.
+    inRunUntilAllBlocked
+        // some other thread or caller is already in the process of closing
+        || closeStarted) {


Probably not much of an improvement except for readability, but you could get rid of closeStarted, make closeFuture nullable, only create the future at 331, and change this to || closeFuture != null.

Nah, the problem is that we can be here if inRunUntilAllBlocked is true, closeStarted is false. In this case, we don't have a future to wait on. We also can't create one if we use closeFuture != null as closeStarted.

I think it's also easier for future maintenance of this class to have a final CompletableFuture representing closure from the start that can be referenced at any moment at all even without taking a lock.

Concur on the final (though could use AtomicReference I guess). I just know that they have similar use. But defniitely a subjective thing and adds no real value. Ignore.

cretz · 2022-06-09T15:08:23Z

temporal-sdk/src/main/java/io/temporal/internal/sync/DeterministicRunnerImpl.java

@@ -343,6 +372,9 @@ public void close() {
        throw new Error("Unexpected failure stopping coroutine", e);
      }
    }
+
+    closeFuture.complete(null);
+    closed = true;


Javadoc says this is to be used under lock, but this is section of code is not locked (can make synchronized or use AtomicBoolean if it's important to be unlocked while waiting on threadFutures)

Also, some languages have compile-time guards you can use with annotations that, when you say a mutex/lock applies to a class member, it statically analyzes that only that class member is used under that lock. Does Java have anything like that?

Good catch! Yeah. I think, instead of making it Atomic, I will better just get rid of a separate var altogether and will just use the future instead.

Spikhalskiy requested review from mfateev, Sushisource, cretz, bergundy and mmcshane as code owners June 8, 2022 20:17

Spikhalskiy marked this pull request as draft June 8, 2022 20:17

Spikhalskiy force-pushed the runner-close-blocks branch 3 times, most recently from 0e1d01d to 054bb25 Compare June 9, 2022 05:08

Spikhalskiy marked this pull request as ready for review June 9, 2022 05:25

Spikhalskiy force-pushed the runner-close-blocks branch from 054bb25 to 5a2f4b7 Compare June 9, 2022 13:50

Spikhalskiy commented Jun 9, 2022

View reviewed changes

cretz reviewed Jun 9, 2022

View reviewed changes

Spikhalskiy force-pushed the runner-close-blocks branch 2 times, most recently from 4adfb85 to 781287a Compare June 9, 2022 16:14

cretz approved these changes Jun 9, 2022

View reviewed changes

Make DeterministicRunner#close always block until the closing is done

5f9f192

Spikhalskiy force-pushed the runner-close-blocks branch from 781287a to 5f9f192 Compare June 9, 2022 17:41

Spikhalskiy merged commit 1b1c1d4 into temporalio:master Jun 9, 2022

Spikhalskiy deleted the runner-close-blocks branch June 9, 2022 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DeterministicRunner#close always block until the closing is done #1255

Make DeterministicRunner#close always block until the closing is done #1255

Spikhalskiy commented Jun 8, 2022 •

edited

Loading

Spikhalskiy Jun 9, 2022

cretz Jun 9, 2022

Spikhalskiy Jun 9, 2022

cretz Jun 9, 2022

Spikhalskiy Jun 9, 2022 •

edited

Loading

cretz Jun 9, 2022

Spikhalskiy Jun 9, 2022 •

edited

Loading

cretz Jun 9, 2022

Spikhalskiy Jun 9, 2022 •

edited

Loading

cretz Jun 9, 2022

Spikhalskiy Jun 9, 2022

cretz Jun 9, 2022

cretz Jun 9, 2022

Spikhalskiy Jun 9, 2022

Make DeterministicRunner#close always block until the closing is done #1255

Make DeterministicRunner#close always block until the closing is done #1255

Conversation

Spikhalskiy commented Jun 8, 2022 • edited Loading

What was changed

Why?

How was this tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy commented Jun 8, 2022 •

edited

Loading

Spikhalskiy Jun 9, 2022 •

edited

Loading

Spikhalskiy Jun 9, 2022 •

edited

Loading

Spikhalskiy Jun 9, 2022 •

edited

Loading