zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-06-27 14:54:51 +02:00
Code Issues Projects Releases Packages Wiki Activity

Fix flow double-init bug by removing @FlowScope on provideFlow()

Browse source 
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<Flow>).  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
This commit is contained in:
nickfelt 2016-06-29 13:25:39 -07:00 • committed by Ben McIlwain
parent 65f920594b
commit 01698059f6
1 changed files with 3 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

4
java/google/registry/flows/FlowComponent.java
View file

@ -118,8 +118,10 @@ public interface FlowComponent {
/** Module to delegate injection of a desired {@link Flow}. */ /** Module to delegate injection of a desired {@link Flow}. */
@Module @Module
static class FlowComponentModule { static class FlowComponentModule {
// WARNING: @FlowScope is intentionally omitted here so that we get a fresh Flow instance on
// each call to Provider<Flow>.get(), to avoid Flow instance re-use upon transaction retries.
// TODO(b/29874464): fix this in a cleaner way.
@Provides @Provides
@FlowScope
static Flow provideFlow(FlowComponent flows, Class<? extends Flow> clazz) { static Flow provideFlow(FlowComponent flows, Class<? extends Flow> clazz) {
return clazz.equals(ContactCheckFlow.class) ? flows.contactCheckFlow() return clazz.equals(ContactCheckFlow.class) ? flows.contactCheckFlow()
: clazz.equals(ContactCreateFlow.class) ? flows.contactCreateFlow() : clazz.equals(ContactCreateFlow.class) ? flows.contactCreateFlow()