Fix #1354 - Isolate variables in ForExecutor to avoid racing condition to overwrite loop variables#1363
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1354 by changing how ForExecutor handles loop variables to prevent async tasks (e.g., emit) from observing overwritten $item/$index values. It also refactors supporting test utilities by making InMemoryEvents subclass-friendly, introducing a lagged event broker for deterministic async tests, and relocating TraceExecutionListener into impl code for reuse.
Changes:
- Update
ForExecutorto capture per-iteration loop values and reapply them at execution time in the async chain. - Make
InMemoryEventsfields protected/final to support subclassing; addLaggedInMemoryEventsfor regression testing async behavior. - Move
TraceExecutionListenerintoio.serverlessworkflow.impl.lifecycleand update tests/tools to import it from the new package.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| impl/test/src/test/java/io/serverlessworkflow/impl/test/MvStorePersistenceTest.java | Update import to new TraceExecutionListener package. |
| impl/test/src/test/java/io/serverlessworkflow/impl/test/DBGenerator.java | Update import to new TraceExecutionListener package. |
| impl/core/src/main/java/io/serverlessworkflow/impl/lifecycle/TraceExecutionListener.java | Change package to impl.lifecycle (and remove now-redundant imports). |
| impl/core/src/main/java/io/serverlessworkflow/impl/executors/ForExecutor.java | Attempt to fix foreach race by capturing item/index and re-setting variables in the async callback. |
| impl/core/src/main/java/io/serverlessworkflow/impl/events/InMemoryEvents.java | Expose internals as protected to allow inheritance in tests/client code. |
| experimental/test/src/test/java/io/serverlessworkflow/fluent/test/LaggedInMemoryEvents.java | New lagged publisher used to make async foreach behavior reproducible. |
| experimental/test/src/test/java/io/serverlessworkflow/fluent/test/ForEachFuncTest.java | Switch to lagged broker and add listener to help validate the foreach/emit regression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fjtirado
left a comment
There was a problem hiding this comment.
Please put back traceExecutionListerns as test class.
This will avoid unrelated change in DBGbenrator and MvStorePersistenceTest.
|
@ricardozanini Nice pr that fixes that particular race condition, but I realized that ForExecutor was not properly implemented, there are more race conditions and opened this alternate one, please take a look |
…d for multithread Signed-off-by: fjtirado <ftirados@redhat.com>
Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>
…id racing condition to overwrite loop variables Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>
Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>
…d for multithread Signed-off-by: fjtirado <ftirados@redhat.com>
2e95a6c to
19f8c9b
Compare
Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it:
Fixes #1354
Special notes for reviewers:
InMemoryEventsclass slightly to be inherit-friendlyTranceExecutionListenertoimplso it can easily be reused in other tests and in client code - it's quite useful for debugging/tracing, even in prod code.Additional information (if needed):