From d3e71b7f35abd3e71548d963d04cc8b6d7ca697d Mon Sep 17 00:00:00 2001
From: Steven Littiebrant <imgroxx@gmail.com>
Date: Mon, 6 Aug 2018 17:45:31 -0700
Subject: [PATCH] Bug: inferred func names are ambiguous, cause misbehavior due
 to alias

`RegisterActivityWithOptions` registers an inferred func name into a
map containing `{inferred-name: given-name}`, I assume to allow you to
execute activities with either their given-name or their function, for
simplicity.

Unfortunately this is unsafe - reflected function names are not guaranteed
to be unique.  And it's relatively easy to encounter if you generate
workflow or activity functions (as can easily occur in tests, and could
occur IRL as well, though generally you'd just pass those args to a shared
activity).

---

TBH I'm not fond of allowing ambiguous calls like this enables, but that's
mostly irrelevant.  This silently produces incorrect behavior, which it
should not do.

Making it error on duplicate-inferred-names would prevent a silent failure...
but it'd also mean you cannot generate activities / workflows at all.
And trying to allow generated activities / workflows while also allowing
ambiguous calls doesn't seem easily possible... func addresses would work
for anonymous funcs, but for named ones you can only get the address of the
var, not the "canonical" one.  Maybe something with reflection -> function
pointer address?

I dunno.  I submit this as evidence of bad behavior, and to discuss what it
means for the future.
---
 internal/internal_workflow_testsuite_test.go | 37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/internal/internal_workflow_testsuite_test.go b/internal/internal_workflow_testsuite_test.go
index 0024358f7..463f2f390 100644
--- a/internal/internal_workflow_testsuite_test.go
+++ b/internal/internal_workflow_testsuite_test.go
@@ -1792,3 +1792,40 @@ func (s *WorkflowTestSuiteUnitTest) Test_ContextMisuse() {
 	s.Error(env.GetWorkflowError())
 	s.Contains(env.GetWorkflowError().Error(), "block on coroutine which is already blocked")
 }
+
+func (s *WorkflowTestSuiteUnitTest) Test_AmbiguousRegistration() {
+	workflowFn := func(ctx Context) error {
+		ctx = WithActivityOptions(ctx, ActivityOptions{
+			ScheduleToStartTimeout: time.Minute,
+			StartToCloseTimeout:    time.Minute,
+		})
+		return ExecuteActivity(ctx, f1).Get(ctx, nil)
+	}
+	env := s.NewTestWorkflowEnvironment()
+	RegisterWorkflow(workflowFn)
+
+	env.OnActivity("f1").Return(errors.New("f1 called"))
+	env.OnActivity("f2").Return(errors.New("incorrect behavior, f2 called"))
+
+	env.ExecuteWorkflow(workflowFn)
+
+	s.True(env.IsWorkflowCompleted())
+	s.Error(env.GetWorkflowError())
+
+	// note that this will fail - f2 was called.
+	// if you swap f1 and f2 registers in the init below, it'll pass, because the second write wins.
+	s.Contains(env.GetWorkflowError().Error(), "f1 called")
+}
+
+func init() {
+	f1 = reg("f1")
+	f2 = reg("f2")
+}
+
+var f1, f2 func() error
+
+func reg(name string) func() error {
+	f := func() error { return nil }
+	RegisterActivityWithOptions(f, RegisterActivityOptions{name})
+	return f
+}