Proposal: Provide context.Context via goka.Context

[burdiyan]

Oftentimes during message processing firing another IO operation is needed (like querying a remote database, or executing remote HTTP call, or similar). Normally these IO operation would accept context.Context for cancellation.

It would be great if Goka could propagate the context passed to processor's Run method to the actual callback.

One of the way to do it in a backward-compatible way could be embedding context.Context interface inside goka.Context directly, so that goka.Context satisfies context.Context. Then one could pass goka.Context directly to the IO function or/and create child context.Context off of it.

My only concern about it is that goka.Context would do even more things, and it's already doing a lot. But as a benefit of this solution we could avoid confusion between context.Context and goka.Context as use of context.Context is becoming more wide in the community.

[db7]

I agree that we need to pass the context.Context somehow up to the ProcessCallback. Otherwise one needs to hack it via closure.

Another problem of extending the goka.Context interface is that it conflicts with the context.Context() in the Value() method.

Do you think that returning the context with ctx.Context() or ctx.Background() would be an alternative? Too confusing? Otherwise we'd need to rename the Value() method.

[burdiyan]

You're right about the conflict in the method name! If backward compatibility is not an issue, Value could be renamed. Although I think we could do something similar to what net/http did for Request:

1. Add Context method to goka.Context. Calling this method would return context.Context passed to the processor's Run method.
2. Add WithContext method to goka.Context. It would accept context.Context and would return shallow copy of goka.Context. Like that: https://godoc.org/net/http#Request.WithContext This is useful for creating child context off of processor's context. For example to add some trace id for a particular message or stuff like this.

I think this kind of change would be the least destructive.

[burdiyan]

I forgot to mention that step 0 would be to insert processor's context.Context inside goka.Context, that further would be available with Context() method on goka.Context.

[db7]

Steps 0 and 1 are very easy, I can quickly build them. But I am not very sure how one would use the WithContext method. Can you give an example?

Looking forward, after closing this ticket with a non-destructive solution, I'd suggest we start discussing how the callback interface could be redesigned. We can plan an API breaking release if we pack enough changes together, for example, #108.

[burdiyan]

WithContext could be useful if you isolate the logic in some sort of Service struct or separate function.

For example I commonly implement the handling in some sort of service type, and then wrap it inside DefineGroup call.

Here is a rough example:

func NewProcessor(brokers []string, svc *FooService, opts ...goka.ProcessorOption) {
	goka.DefineGroup("group", goka.Input("topic1", new(myCodec), func(ctx goka.Context, message interface{}) {
		// Here is when we could derive context if needed...
		stdctx := addTracingInformationOrAnythingElse(ctx.Context())
		ctx = ctx.WithContext(stdctx)

		if err := svc.HandleFooMessage(ctx, message.(*FooMessage)); err != nil {
			// Sometimes not all errors should trigger panic, so we can handle this specific errors in one place here if needed.
			// ...

			ctx.Fail(err)
		}
	}))
}

type FooService struct {
	Log Logger
	DB Database
	Client SomeHTTPClient
	// ...
}

type FooMessage struct {
	FieldOne string
	FieldTwo string
	// ...
}

func (fs *FooService) HandleFooMessage(ctx goka.Context, message *FooMessage) error {
	// Here is where one could use the underlying context.
	fs.Client.Do(ctx.Context(), "rest of parameters here")

	// Handle specific type of *FooMessage.
	// Don't use ctx.Fail here, and instead return normal error.
	// This is more portable and easier to test if needed to test only the logic.
	return nil
}

[burdiyan]

Totally agree on that. We could group enough breaking changes before doing them.

Looking forward, after closing this ticket with a non-destructive solution, I'd suggest we start discussing how the callback interface could be redesigned. We can plan an API breaking release if we pack enough changes together, for example, #108.

[frairon]

Let's close this and have another discussion if the issue comes up again.