From 01698059f61357c0cd9c5a13df305a004a24197f Mon Sep 17 00:00:00 2001 From: nickfelt Date: Wed, 29 Jun 2016 13:25:39 -0700 Subject: [PATCH] Fix flow double-init bug by removing @FlowScope on provideFlow() See [] for details, but basically, @FlowScope causes the Flow instance produced by flowProvider.get() in FlowRunner to be the same each time it's called, which leads to the instance being re-used when a transactional retry (e.g. for a ConcurrentModificationException) causes a flow to be attempted more than once. Flow is not meant to be re-used and certain flows fail at runtime when this happens, so the effect is that a CME now aborts most EPP requests, which is bad. This is a bit of a hacky fix; finding a better one is tracked in [] == TESTING == This is very hard to test because there isn't really a clean way to trigger a CME from within a flow's execution without hardcoding in assumptions about what a given flow is doing when it runs, and we can't easily supply a custom Flow for testing while also exercising the Flow daggerization process (since this bug only appears due to the specific way that dagger constructs the Provider). Ideally a fix would improve the testability here as well. For now, I've manually tested this change by pasting code into FlowRunner that explicitly throws a ConcurrentModificationException after running the flow (similar to DryRunException), but only on the first transaction attempt. With @FlowScope on provideFlow(), this change reproduces the UnsupportedOperationException issue in many tests; once it's removed (i.e. with this CL submitted) the problem goes away. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=126226066 --- java/google/registry/flows/FlowComponent.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/java/google/registry/flows/FlowComponent.java b/java/google/registry/flows/FlowComponent.java index 2961c0b3c..8b06c8487 100644 --- a/java/google/registry/flows/FlowComponent.java +++ b/java/google/registry/flows/FlowComponent.java @@ -118,8 +118,10 @@ public interface FlowComponent { /** Module to delegate injection of a desired {@link Flow}. */ @Module static class FlowComponentModule { + // WARNING: @FlowScope is intentionally omitted here so that we get a fresh Flow instance on + // each call to Provider.get(), to avoid Flow instance re-use upon transaction retries. + // TODO(b/29874464): fix this in a cleaner way. @Provides - @FlowScope static Flow provideFlow(FlowComponent flows, Class clazz) { return clazz.equals(ContactCheckFlow.class) ? flows.contactCheckFlow() : clazz.equals(ContactCreateFlow.class) ? flows.contactCreateFlow()