Replace DeclarativeRecipe ThreadLocal accumulator with cursor-based lookup#6936
Closed
pstreef wants to merge 2 commits intoopenrewrite:mainfrom
Closed
Replace DeclarativeRecipe ThreadLocal accumulator with cursor-based lookup#6936pstreef wants to merge 2 commits intoopenrewrite:mainfrom
pstreef wants to merge 2 commits intoopenrewrite:mainfrom
Conversation
…veRecipe PR openrewrite#6810 changed DeclarativeRecipe.accumulator from a plain field to ThreadLocal to fix concurrent recipe runs overwriting each other's accumulator. However, ThreadLocal values are lost when execution resumes on a different thread after yielding. The accumulator is already stored on the rootCursor by ScanningRecipe.getAccumulator(), which is per-run and survives thread switches. This change removes the redundant ThreadLocal storage and instead resolves the accumulator from the rootCursor at visit time. The PreconditionBellwether now defers precondition resolution to visit time (when the rootCursor is available) rather than eagerly resolving in a field initializer.
Each getVisitor() call creates a new single-threaded instance, so caching the resolved visitor within it is safe and avoids redundant precondition tree walks on every isAcceptable/visit call.
Contributor
Author
|
Closing this — after deeper investigation of the worker's yield mechanics, the ThreadLocal approach from #6810 is correct. The rootCursor is preserved across yield boundaries within the same cycle, and scan/edit happen within the same RecipeRunCycle batch on the same thread. The ThreadLocal is set during scan and read during edit of the same batch. On resume after yield, a new batch does its own scan phase which sets the ThreadLocal fresh. The actual fix for the yield-related accumulator loss was #6913 (delegating getAccumulator in BellwetherDecoratedScanningRecipe to the wrapped recipe's stable UUID). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DeclarativeRecipe.accumulatorfrom a plain field toThreadLocal<Accumulator>to fix concurrent recipe runs overwriting each other's accumulator. However, when a recipe run yields and resumes on a different thread, the ThreadLocal value is lost.The
orVisitors()method reads the accumulator from the ThreadLocal to resolve scanning precondition visitors. After a thread switch,accumulator.get()returnsnull, causing scanning preconditions to receive null accumulators. This affects anyDeclarativeRecipethat uses aScanningRecipeas a precondition.Solution
The accumulator is already stored on the rootCursor by
ScanningRecipe.getAccumulator(), which is per-run and survives thread switches. The ThreadLocal was redundant secondary storage.ThreadLocal<Accumulator>entirelysetCursor()set byRecipeRunCycle)orVisitors()reads the accumulator from the cursor viagetAccumulator(rootCursor, ctx)instead of ThreadLocalonComplete()ThreadLocal cleanup and simplifyclone()