Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support context propagation on overloads #558

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nlachfr
Copy link
Contributor

@nlachfr nlachfr commented Jun 26, 2022

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

@leaxoy
Copy link

leaxoy commented Oct 16, 2023

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

@TristonianJones
Copy link
Collaborator

@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.

@nlachfr
Copy link
Contributor Author

nlachfr commented Oct 19, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support context propagation on overloads
3 participants