cel-go icon indicating copy to clipboard operation
cel-go copied to clipboard

Support context propagation on overloads

Open nlachfr opened this issue 3 years ago • 3 comments

These changes introduce :

  • a new interface functions.Overloader for keeping compatiblity with already defined functions.Overload structures
  • a new structure functions.ContextOverload for defining context capable overloads
  • the extension of the interpreter.Activation interface with context.Context
  • a small test for checking that context cancellation can be catched with underlying overloads

The idea was to keep API compatibility while propagating the execution context to overload functions.

Resolves #557

nlachfr avatar Jun 26 '22 14:06 nlachfr

This seems reasonable. The only question is, when can it be merged?

leaxoy avatar Oct 16 '23 03:10 leaxoy

@leaxoy storing the Context as a member on a struct is an anti-pattern in Go, so I'd need to find another way to pass this information along. I had some thoughts about how to approach this with the async eval draft PR, but haven't had a chance to return to it.

TristonianJones avatar Oct 16 '23 15:10 TristonianJones

In #557, you mentioned the use of the callback channel directly.

Also, in Kubernetes it seems to be more common to store the callback channel.

Just propagating the done channel to function overloads would at least allow cancellation propagation (which was the initial objective of this MR), while removing the context embedding. We could then have a wrapper of the channel respecting context.Context interface for overloads.

I might be able to propose something for this if you're good with that @TristonianJones

nlachfr avatar Oct 19 '23 09:10 nlachfr