Skip to content

Commit e3a8798

Browse files
Replace ThreadLocal accumulator with cursor-based lookup in DeclarativeRecipe (#7225)
When a ScanningRecipe is used as a precondition in a DeclarativeRecipe, the accumulator was stored in a ThreadLocal during the scan phase and read back in orVisitors() during the edit phase. Worker yielding could cause the edit phase to resume on a different thread, where the ThreadLocal was null, resulting in NPE. The fix uses the existing cursor-based accumulator storage mechanism (ScanningRecipe.getAccumulator) which stores accumulators on the root cursor keyed by UUID. This survives across threads since the cursor is passed through RecipeRunCycle boundaries. Changes: - Remove ThreadLocal<Accumulator> field - PreconditionBellwether now lazily resolves precondition visitors in visit() where cursor and ExecutionContext are available - orVisitors() takes Cursor + ExecutionContext parameters and uses getAccumulator(rootCursor, ctx) instead of ThreadLocal - Remove onComplete() that only cleared the ThreadLocal - Simplify clone() which only copied the ThreadLocal Fixes #7159
1 parent 4032a63 commit e3a8798

1 file changed

Lines changed: 28 additions & 30 deletions

File tree

rewrite-core/src/main/java/org/openrewrite/config/DeclarativeRecipe.java

Lines changed: 28 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import java.time.Duration;
2727
import java.util.*;
2828
import java.util.function.Function;
29-
import java.util.function.Supplier;
3029

3130
import static java.util.Collections.emptyList;
3231
import static org.openrewrite.Validated.invalid;
@@ -162,16 +161,12 @@ private void initializeDeclarativeRecipe(DeclarativeRecipe declarativeRecipe, St
162161
}
163162
}
164163

165-
@JsonIgnore
166-
private transient ThreadLocal<Accumulator> accumulator = new ThreadLocal<>();
167-
168164
@Override
169165
public Accumulator getInitialValue(ExecutionContext ctx) {
170166
Accumulator acc = new Accumulator();
171167
for (Recipe precondition : preconditions) {
172168
registerNestedScanningRecipes(precondition, acc, ctx);
173169
}
174-
accumulator.set(acc);
175170
return acc;
176171
}
177172

@@ -223,27 +218,43 @@ static class PreconditionBellwether extends Recipe {
223218
String description = "Evaluates a precondition and makes that result available to the preconditions of other recipes. " +
224219
"\"bellwether\", noun - One that serves as a leader or as a leading indicator of future trends.";
225220

226-
Supplier<TreeVisitor<?, ExecutionContext>> precondition;
221+
DeclarativeRecipe declarativeRecipe;
227222

228223
@NonFinal
229224
transient boolean preconditionApplicable;
230225

231226
@Override
232227
public TreeVisitor<?, ExecutionContext> getVisitor() {
233228
return new TreeVisitor<Tree, ExecutionContext>() {
234-
final TreeVisitor<?, ExecutionContext> p = precondition.get();
229+
@Nullable
230+
TreeVisitor<?, ExecutionContext> p;
235231

236232
@Override
237233
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
238-
return p.isAcceptable(sourceFile, ctx);
234+
// p is lazily resolved in visit() where the cursor is available.
235+
// Before that, conservatively accept all source files.
236+
return p == null || p.isAcceptable(sourceFile, ctx);
239237
}
240238

241239
@Override
242240
public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
243-
Tree t = p.visit(tree, ctx);
241+
Tree t = resolve(ctx).visit(tree, ctx);
244242
preconditionApplicable = t != tree;
245243
return tree;
246244
}
245+
246+
private TreeVisitor<?, ExecutionContext> resolve(ExecutionContext ctx) {
247+
if (p == null) {
248+
Cursor rootCursor = getCursor().getRoot();
249+
List<TreeVisitor<?, ExecutionContext>> andVisitors = new ArrayList<>();
250+
for (Recipe precondition : declarativeRecipe.preconditions) {
251+
andVisitors.add(declarativeRecipe.orVisitors(precondition, rootCursor, ctx));
252+
}
253+
//noinspection unchecked
254+
p = Preconditions.and(andVisitors.toArray(new TreeVisitor[0]));
255+
}
256+
return p;
257+
}
247258
};
248259
}
249260
}
@@ -496,36 +507,30 @@ public final List<Recipe> getRecipeList() {
496507
return recipeList;
497508
}
498509

499-
List<Supplier<TreeVisitor<?, ExecutionContext>>> andPreconditions = new ArrayList<>();
500-
for (Recipe precondition : preconditions) {
501-
andPreconditions.add(() -> orVisitors(precondition));
502-
}
503-
//noinspection unchecked
504-
PreconditionBellwether bellwether = new PreconditionBellwether(Preconditions.and(andPreconditions.toArray(new Supplier[]{})));
510+
PreconditionBellwether bellwether = new PreconditionBellwether(this);
505511
List<Recipe> recipeListWithBellwether = new ArrayList<>(recipeList.size() + 1);
506512
recipeListWithBellwether.add(bellwether);
507513
recipeListWithBellwether.addAll(decorateWithPreconditionBellwether(bellwether, recipeList));
508514
return recipeListWithBellwether;
509515
}
510516

511-
private TreeVisitor<?, ExecutionContext> orVisitors(Recipe recipe) {
517+
@SuppressWarnings({"rawtypes", "unchecked"})
518+
private TreeVisitor<?, ExecutionContext> orVisitors(Recipe recipe, Cursor rootCursor, ExecutionContext ctx) {
512519
List<TreeVisitor<?, ExecutionContext>> conditions = new ArrayList<>();
513520
if (recipe instanceof ScanningRecipe) {
514-
//noinspection rawtypes
515521
ScanningRecipe scanning = (ScanningRecipe) recipe;
516-
//noinspection unchecked
517-
Accumulator acc = accumulator.get();
518-
conditions.add(scanning.getVisitor(acc != null ? acc.recipeToAccumulator.get(scanning) : null));
522+
Accumulator acc = getAccumulator(rootCursor, ctx);
523+
Object recipeAcc = acc.recipeToAccumulator.get(scanning);
524+
conditions.add(scanning.getVisitor(recipeAcc));
519525
} else {
520526
conditions.add(recipe.getVisitor());
521527
}
522528
for (Recipe r : recipe.getRecipeList()) {
523-
conditions.add(orVisitors(r));
529+
conditions.add(orVisitors(r, rootCursor, ctx));
524530
}
525531
if (conditions.size() == 1) {
526532
return conditions.get(0);
527533
}
528-
//noinspection unchecked
529534
return Preconditions.or(conditions.toArray(new TreeVisitor[0]));
530535
}
531536

@@ -608,16 +613,9 @@ private static class LazyLoadedRecipe extends Recipe {
608613
String description = "Recipe that is loaded lazily.";
609614
}
610615

611-
@Override
612-
public void onComplete(ExecutionContext ctx) {
613-
accumulator.remove();
614-
}
615-
616616
@Override
617617
public DeclarativeRecipe clone() {
618-
DeclarativeRecipe cloned = (DeclarativeRecipe) super.clone();
619-
cloned.accumulator = new ThreadLocal<>();
620-
return cloned;
618+
return (DeclarativeRecipe) super.clone();
621619
}
622620

623621
@Override

0 commit comments

Comments
 (0)