DefaultActorClock is not thread safe

[Zelldon]

Describe the bug

It seems we now create the ActorScheduler with a predefined actor clock, either ControlledClock or the DefaultActorClock https://github.com/camunda/zeebe/blob/main/dist/src/main/java/io/camunda/zeebe/shared/ActorClockConfiguration.java#L34-L35

The problem is that the DefaultActorClock is not thread safe! Previously we created the ActorScheduler without clock, which caused that each thread has its own clock. Then there is no issue.

If we use the DefaultActorClock in all threads this might cause issues on updating the time https://github.com/camunda/zeebe/blob/main/scheduler/src/main/java/io/camunda/zeebe/scheduler/clock/DefaultActorClock.java#L24-L31 which is not thread safe and called by all threads https://github.com/camunda/zeebe/blob/main/scheduler/src/main/java/io/camunda/zeebe/scheduler/ActorThread.java#L78

and also when timers are scheduled https://github.com/camunda/zeebe/blob/main/scheduler/src/main/java/io/camunda/zeebe/scheduler/ActorTimerQueue.java#L53

I found that during investigating #10390 and trying to make ActorTimerQueue threadsafe...

To Reproduce

Expected behavior

Either we not set the clock or we use a thread safe structure.

Log/Stacktrace

Full Stacktrace

<STACKTRACE>

Environment:

• OS:
• Zeebe Version: 8.x
• Configuration:

[Zelldon]

Right now I can't really estimate the impact, but I guess it might cause issues in executing scheduled tasks 🤷

[npepinpe]

Oh, damn, good catch! I didn't think of that. We could do two approaches, one is making the clock thread-safe, the other is simply going back to the original behavior with an asterisk: you can inject a pre-made clock (which is then used by all threads), or nothing, which then means all threads use their own clock. I think there's something to be said about the safety of having all threads use the same source of truth for time, but I'm also not sure what the impact is when they don't - we never really noticed clock skew/drift before (though, would we have anyway until it becomes a lot? 🤷).

[npepinpe]

Looking at it, I think the impact is quite volatile. It could be limited, but it could also be terrible if you hit some thread scheduling snag (like a thread being suspended right before updating the timeMillis, then for some reason no scheduled for 10 seconds, resetting that clock backwards 10 seconds).

So we should 100% make it thread-safe or go with the "let every thread create it when not provided" approach.

[lenaschoenburg]

Maybe we could look into using InstantSource which is threadsafe and supports custom tick durations.

[npepinpe]

I thought about it (well, the impl, Clock#tick), but its slightly different from what I understand. It doesn't cache anything but rather just truncated the result so it looks like it's ticking every milli only. Unless InstantSource is different?

[Zelldon]

What do you folks think of first (as quick fix before the release) we go with not setting the actor clock, which means DefaultActorClock for each thread. After the release we can experiment with making it thread safe?

[npepinpe]

Sounds good to me 👍

[megglos]

Issue got mitigated with #10489

Remaining impact is not severe.

We decided to close it for now as we don't thin it's a big deal going forward.