Add ValidateContext for cancellation and timeout support#327
Conversation
|
@pkwarren Would you like to give it a shot? 😄 |
|
Hi, @tonobo, thanks for the patch! I'd like to dig into your use-case here, could you share some details on the size of the message you're validating where cancellation is relevant? What type of validation rules are you using that are this CPU intensive? It's cool to see that cel-go added support for context-aware execution, but I'm wary of foisting this behavior onto all users of this library when most messages are evaluated on the order of a microsecond. I'd love it if you could add benchmarks to show that this does not regress the default usage. |
|
Hey, great to hear someone is interested in this contribution! First, the use case: imagine a global repository managing all contracts, so To make this concrete, here is an example. A message holds a list of named sets plus several rule lists that reference message Firewall {
option (buf.validate.message).cel = {
id: "ipset_keys_unique"
message: "ipset keys must be unique within this firewall"
expression: "this.ipsets.map(s, string(s.key.type) + ':' + s.key.name).unique()"
};
// one of these exists per rule list (ingress/egress, several stages),
// so this pattern repeats 4+ times on the same message
option (buf.validate.message).cel = {
id: "ingress_ipsets_exist"
message: "all ingress rules must only reference ipsets which exist within this firewall"
expression:
"this.ingress.rules.all(rule, rule.remote_ipsets.all(key,"
"this.ipsets.exists(ipset, ipset.key.type == key.type && ipset.key.name == key.name)))"
};
// ...
}Each of those rules is O(rules * refs_per_rule * ipsets), so validation cost CEL provides different options to tackle this. On one end, you can set In conclusion: protovalidate is used to enforce contracts between parties, Regarding the benchmarks: fair point! I'll attach a benchmark test comparing |
Introduce a context-aware ValidateContext(ctx, msg, options...) alongside the existing Validate(), so callers can bound long or expensive validation with a context deadline or cancellation. The motivating case is complex validation work such as CEL rules over large collections. Content: - Add a ContextValidator interface (Validator plus ValidateContext). New now returns ContextValidator, so New() callers get ValidateContext directly while still satisfying Validator. The change is backward compatible: the Validator interface is unchanged, so external implementations of it keep compiling. - Add the package-level ValidateContext helper and implement ValidateContext on the global validator. - Thread context through every evaluator via parallel *Context methods. - Evaluate CEL through cel-go's ContextEval and enable InterruptCheckFrequency so a long-running expression can be interrupted mid-evaluation. - Check ctx.Err() at message entry and in the repeated/map iteration loops so native validation of large messages also honors cancellation. - Return the raw context error (context.Canceled / DeadlineExceeded), never wrapped, so callers can match it with errors.Is. Keeping Validate() free of overhead: A context that can never be cancelled (context.Background) has a nil Done channel. ValidateContext detects this once per call and selects one of two validation configs built at New(); evaluation then skips every per-node ctx.Err() check and calls cel-go's Eval rather than ContextEval, whose activation wrapping would otherwise be paid on every expression. Validate() therefore takes the same path it did before context support existed, and neither entry point allocates to carry the flag. Choosing the interrupt check frequency: cel-go only checks for cancellation inside comprehension loops (all, exists, map, filter), once per iteration, and never for expressions without a comprehension. The frequency does not skip that per-iteration bookkeeping; it only gates a cheap non-blocking poll of ctx.Done(). Raising it therefore buys no measurable throughput while making cancellation latency proportionally worse: on the BenchCrossReference fixture a 50ms deadline is honored in 50ms at frequency 1, but takes 2.2s at frequency 100. The default is therefore 1, and WithCELInterruptCheckFrequency lets callers tune or disable it (0). Benchmarks: - BenchmarkContextOverhead compares Validate, ValidateContext with a background context, and ValidateContext with a cancellable context. - BenchmarkInterruptCheckFrequency shows throughput is flat across frequencies. - BenchCrossReference is a new message whose CEL rule costs O(n^3), used to test that an expensive expression is actually interrupted by a deadline.
c9a35a6 to
0686058
Compare
|
|
@rodaine Is there anything else I can do for you? It would be great if we could make some progress here :D Sorry for bugging you. 🙈 |
pkwarren
left a comment
There was a problem hiding this comment.
Thank you for adding some benchmarks. It would be good to gather benchstat output from before/after this change to ensure no regressions.
Overall the changes look good but this should be opt-in behavior for users with expensive validation constraints.
| // frequency only gates the (very cheap) non-blocking channel poll. Raising it | ||
| // therefore buys no measurable throughput, while making cancellation latency | ||
| // proportionally worse. 1 is the responsive default. | ||
| const DefaultInterruptCheckFrequency = 1 |
There was a problem hiding this comment.
We should remove this constant and interrupt checks should be disabled by default (matching CEL's defaults). To this point no users of protovalidate-go have requested this feature and shouldn't require this additional overhead by default. We can always re-evaluate the default later.
| Evaluate(msg protoreflect.Message, val protoreflect.Value, cfg *validationConfig) error | ||
|
|
||
| // EvaluateContext is like Evaluate, but honors context cancellation. | ||
| EvaluateContext(ctx context.Context, msg protoreflect.Message, val protoreflect.Value, cfg *validationConfig) error |
There was a problem hiding this comment.
Instead of overloads on the evaluator (internal interface) all the way down we should just update the signature for Evaluate to take a ctx context.Context.
|
|
||
| // EvaluateMessageContext is like EvaluateMessage, but honors context | ||
| // cancellation. | ||
| EvaluateMessageContext(ctx context.Context, msg protoreflect.Message, cfg *validationConfig) error |
There was a problem hiding this comment.
Same here for EvaluateMessage.
|
|
||
| func (m *kvPairs) evalPairs(msg protoreflect.Message, key protoreflect.MapKey, value protoreflect.Value, cfg *validationConfig) (err error) { | ||
| evalErr := m.KeyRules.EvaluateField(msg, key.Value(), cfg, true) | ||
| func (m *kvPairs) evalPairsContext(ctx context.Context, msg protoreflect.Message, key protoreflect.MapKey, value protoreflect.Value, cfg *validationConfig) (err error) { |
There was a problem hiding this comment.
Just change the existing method to take a context as the first argument instead of changing every method to add Context suffix.
| val.Map().Range(func(key protoreflect.MapKey, value protoreflect.Value) bool { | ||
| evalErr := m.evalPairs(msg, key, value, cfg) | ||
| if cfg.cancellable { | ||
| if ctxErr := ctx.Err(); ctxErr != nil { |
There was a problem hiding this comment.
We should update these checks to use context.Cause(ctx) instead of ctx.Err().
| return nil | ||
| } | ||
| if cfg.cancellable { | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
Should use context.Cause(ctx) here.
| // is a genuine evaluation error and must keep its RuntimeError identity | ||
| // (with the rule id) even if the context has expired in the meantime. | ||
| if errors.Is(err, interpreter.InterruptError{}) { | ||
| if ctxErr := ctx.Err(); ctxErr != nil { |
|
|
||
| func TestCompiled_ContextCancelled(t *testing.T) { | ||
| t.Parallel() | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
| ctx, cancel := context.WithCancel(context.Background()) | |
| ctx, cancel := context.WithCancel(t.Context()) |
| // | ||
| // ContextValidator is a separate interface (rather than a method on Validator) | ||
| // to preserve backward compatibility for external implementations of Validator. | ||
| type ContextValidator interface { |
There was a problem hiding this comment.
Instead of introducing a new ContextValidator and changing the primary API for users, we should add a ValidationOption named WithContext which takes a context to bound the validate call. The default value would be context.Background() if unset.
The rest of the changes (checking cancelable, passing down a context to evaluators) would largely stay the same.
Add a context-aware ValidateContext(ctx, msg, options...) function alongside Validate() to let callers constrain lengthy or complex validations (e.g., CEL rule evaluation over large collections) using context cancellation or deadlines.
Content: