WorkflowLocal shares initial value between all Workflows

[dano]

Expected Behavior

If I create a WorkflowLocal via WorkflowLocal.withInitial() -> new MyClass()), I get a unique instance of MyClass the first time I call workflowLocal.get() in a given Workflow.

Actual Behavior

I get the same instance of MyClass returned in every Workflow.

Steps to Reproduce the Problem

This test demonstrates the issue:

    @Test
    void test(TestWorkflowEnvironment env, WorkflowClient client, Worker worker) {
        worker.registerWorkflowImplementationFactory(ChildWf.class, () -> () -> {
            var r = ref.get();
            System.out.println(r.get());
        });
        worker.registerWorkflowImplementationFactory(ParentWf.class, () -> () -> {
            var r = ref.get();
            r.set("hi");
            Workflow.newChildWorkflowStub(ChildWf.class).executeChild();
        });
        env.start();
        client.newWorkflowStub(ParentWf.class, WorkflowOptions.newBuilder()
                        .setTaskQueue(worker.getTaskQueue())
                .validateBuildWithDefaults()).execute();
    }

    private static final WorkflowLocal<AtomicReference<String>> ref = WorkflowLocal.withInitial(() -> new AtomicReference<>("bye"));

    @WorkflowInterface
    public interface ParentWf {
        @WorkflowMethod
        void execute();
    }

    @WorkflowInterface
    public interface ChildWf {
        @WorkflowMethod
        void executeChild();
    }

This should print "bye" (which is the 1.19.1 behavior), but instead it prints "hi".

Specifications

• Version: 1.21.1

[dano]

I took a crack at implementing a fix for this here: https://github.com/temporalio/sdk-java/compare/master...dano:sdk-java:fix-wflocal-caching?expand=1

It doesn't break any existing tests, and fixes the issue I'm seeing. If it seems like the right approach, I can clean up the tests a bit, add some additional comments, and open a PR.

[Quinn-With-Two-Ns]

@dano Thanks for implementing a fix, thinking about it a bit more I think caching the result of withInitial even if implemented correctly is a breaking change to WorkflowLocal. Therefore I think we should just remove the caching logic all together.

[dano]

@Quinn-With-Two-Ns do you mean that this test should pass?

WorkflowLocal<AtomicInteger> wf = WorkflowLocal.withInitial(() -> new AtomicInteger(0));
assertNotSame(wf.get(), wf.get());

Meaning that repeatedly calling wf.get() without ever calling set always results in the withInitial callback being re-executed?

It's true that this matches the previous behavior of WorkflowLocal, but I think that behavior was surprising and arguably a bug. ThreadLocal, which this API seems to be emulating, only calls withInitial once for a given thread. Not caching also makes it tricky to use this API correctly. If you're storing a mutable value, you essentially always have use to the API like this to be safe, which is a bit tedious and error-prone:

var myValue = wf.get();
// use myValue here
wf.set(myValue); // Ensure it gets cached.

Otherwise, you risk that the get() call you made returns a value from the withInitial callback that is going to just get thrown away. I actually can't think of a situation where the non-caching behavior is desirable.

That said, it is true that adding caching behavior is a backwards compatibility break. If we must avoid that, even if we agree caching is better behavior, could we add a new factory that implements caching? Something like withInitialCached, or just a new withInitial overload that takes a useCaching boolean?

[Quinn-With-Two-Ns]

do you mean that this test should pass?

correct

I agree with your points, but backwards compatibility it one of the most important parts of the SDK and we prioritize it above almost everything else. Adding an overload for caching or a different function is a backwards compatible change and that would be OK.

[dano]

Understood. I will update my branch to add a new API that uses the caching behavior and open a PR

Would you consider deprecating the non-caching withInitial API, in favor of the new caching version? If so I'll add that to the branch as well.

[Quinn-With-Two-Ns]

Yes I'd be open to deprecating the non-caching API.

Note: we will probably never actually remove the API

[dano]

Thanks, @Quinn-With-Two-Ns. I've opened #1878.

[dano]

Thanks for merging the fix, @Quinn-With-Two-Ns. Do you have a rough idea of when a release that includes this fix will be made?