Bug: inferred func names are ambiguous, cause misbehavior due to alias

[Groxx]

RegisterActivityWithOptions registers an inferred func name into a map containing {inferred-name: registered-name}, I assume to allow you to execute activities with either their registered-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 trigger discussion.

[meiliang86]

Most people will write the code as the following, and it won't be an issue.

func init() {
	RegisterActivityWithOptions(f1, RegisterActivityOptions{"f1"})
	RegisterActivityWithOptions(f2, RegisterActivityOptions{"f2"})
}

func f1() error {
	return nil
}

func f2() error {
	return nil
}

[Groxx]

Part of the reason I bring this up is that it did bite me in some of my code. Most will do that, sure, but not all.

One area where it's particularly common is if you do any dynamic workflow or activity wrappers. I'm adding context info per request to my activity calls, and initializing a per-request cache, and doing so with a reflection-generated func wrapper that looks something like this:

func Register(ctx, fn, name) {
  wrapped := reflect.MakeFunc(reflect.TypeOf(fn), func(in []reflect.Value) []reflect.Value) {
    ctx := context.WithValue(in[0].Interface().(Context), "cache key", cache{})
    in[0] = reflect.ValueOf(ctx)
    return reflect.Call(reflect.ValueOf(fn), in)
  })
  activity.Register(wrapped, RegisterActivityOptions{"name"})
}

which fits the example test exactly. I make a bunch of activities that, as far as reflection is concerned, are all defined at the same location, so they all get the same inferred name.

[CLAassistant]

All committers have signed the CLA.