temporalio / sdk-java

Temporal Java SDK
https://temporal.io
Apache License 2.0
200 stars 134 forks source link

Ensure thread safety of AllOfPromise #2079

Open pivovarit opened 1 month ago

pivovarit commented 1 month ago

What was changed

Replaced the internal AllOfPromise int notReadyCount counter with a thread-safe final AtomicInteger

Why?

AllOfPromise registers multiple Promise instances that were created outside of its scope and uses the handle() method to schedule counter decrements. When the counter reaches zero, it means that all futures completed.

However, handle() method can be executed by different threads, and the internal counter in the current form:

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 1 month ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Quinn-With-Two-Ns commented 1 month ago

@pivovarit all threads in a workflow are managed and coordinated by the SDK, no external threads not managed by the SDK are allowed to be created inside a workflow, so no two threads are ever running concurrently so this should not be necessary .

pivovarit commented 1 month ago

Does it mean that those subsequent handle() callbacks are effectively executed serially by the same thread?

Quinn-With-Two-Ns commented 1 month ago

They are executed serially, potentially by different threads, all with all threads using a common lock for serialization