Fix unsafe modification of Trampoline resources#2031
Conversation
…immediate context
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency bug in Monix’s Trampoline execution machinery where forked/resumed executions could reference and mutate the same non-thread-safe queue across threads, risking concurrent modification and dropped/stuck tasks.
Changes:
- Refactors
Trampolineto resume forked execution on an appropriate trampoline (thread-local on JVM) and to avoid sharing mutable queue state across threads. - Updates
TrampolineExecutionContext(JVM + JS) to use the new trampoline backing abstraction and adds a JVM regression test for nested executions under various schedulers/blocking. - Cleans up
ChunkedArrayQueueAPIs/tests and updates/adds benchmarks related to trampolining and chunked processing.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| monix-execution/shared/src/test/scala/monix/execution/internal/collection/ChunkedArrayQueueSuite.scala | Removes test for removed enqueueAll(Iterable) overload; minor import formatting. |
| monix-execution/shared/src/main/scala/monix/execution/internal/Trampoline.scala | Core fix: resume-on-correct-trampoline behavior and EC modeling to avoid unsafe shared queue mutation. |
| monix-execution/shared/src/main/scala/monix/execution/internal/collection/ChunkedArrayQueue.scala | Removes enqueueAll(Iterable) overload and small iterator visibility tweaks. |
| monix-execution/jvm/src/test/scala/monix/execution/schedulers/TrampolineExecutionContextSuite.scala | Adds JVM regression coverage for nested trampolined executions, blocking, and scheduler variants. |
| monix-execution/jvm/src/main/scala/monix/execution/schedulers/TrampolineExecutionContext.scala | Adapts JVM trampoline EC to new TrampolineEC and thread-local resume trampoline behavior. |
| monix-execution/js/src/main/scala/monix/execution/schedulers/TrampolineExecutionContext.scala | Adapts JS trampoline EC to new TrampolineEC model. |
| benchmarks/shared/src/main/scala/monix/benchmarks/TrampolineExecutionContextBenchmark.scala | Adds new benchmark targeting trampoline + blocking behavior. |
| benchmarks/shared/src/main/scala/monix/benchmarks/TrampolinedRunnableBenchmark.scala | Fixes benchmark instructions and re-enables deep benchmark with corrected loop. |
| benchmarks/shared/src/main/scala/monix/benchmarks/ChunkedMapFilterSumBenchmark.scala | Minor benchmark adjustments (imports + explicit chunk typing). |
| benchmarks/shared/src/main/scala/monix/benchmarks/ChunkedEvalFilterMapSumBenchmark.scala | Minor benchmark adjustments (imports + explicit chunk typing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
FYI, this PR is next on my radar. I'm doing other changes, and it may be cool if we have a way for me to suggest changes, primarily “merge with main” :)) Unsure if you can check the allow edits from maintainers option (although this may mean a security risk for you), otherwise I'll have to either duplicate the PR or make a PR to your original branch. Or we can just sync via comments, but this has a high latency, so whatever works for you, I'm fine with it. |
|
GitHub currently does not support the "Allow edits from maintainers" feature for organization-owned forks. |
|
Apparently this only works for user-owned forks, but I've added @alexandru as external write-level collaborator to this repository. Great to see you back! |
Original contribution from the AVSystem's fork made by @mikkolaj
AVSystem#3