This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-go.git
The following commit(s) were added to refs/heads/main by this push:
new d1a74fbc fix(arrow/compute): do not return a result from a cancelled
call (#1410)
d1a74fbc is described below
commit d1a74fbc9869a373408cfbe8783f01a19428a208
Author: Matt Topol <[email protected]>
AuthorDate: Fri Oct 23 18:34:20 2026 +0100
fix(arrow/compute): do not return a result from a cancelled call (#1410)
### Rationale for this change
`TestCallFunctionPreservesCallerCancellationForVectorFunction` fails
intermittently under `-race` (e.g. ARM64 macOS 14 Go 1.26 - CGO on
#1406):
```
--- FAIL: TestCallFunctionPreservesCallerCancellationForVectorFunction
exec_test.go:385: Expected nil, but got: &compute.ArrayDatum{...}
```
It is a real race, not a test problem: a cancelled `CallFunction` can
return a result with a nil error.
- The kernel gets the **caller's** context: `kctx :=
&exec.KernelCtx{Ctx: ctx, ...}` in `execInternal`.
- `execInternal` then derives a **child** context
(`context.WithCancel(ctx)`), and every cancellation check after that
(`WrapResults`, and the `ctx.Err()` check after it) uses the child.
- Cancelling a context closes its `Done` channel *before* cancelling its
children. In that gap the kernel can wake on the caller's cancellation,
return, and the executor can send its output and close the channel;
`WrapResults` then collects the output while the child context still
reports no error, and `execInternal` returns it.
On main, running that test 20,000 times under `-race` fails 15 times.
### What changes are included in this PR?
`execInternal` keeps a reference to the caller's context and, after
`WrapResults`, treats the call as cancelled if either the caller's or
the child context is done. The returned error is the caller's
cancellation cause when the caller cancelled, otherwise the child's
(unchanged). The kernel, executors and `WrapResults` are unchanged.
### Are these changes tested?
By the existing
`TestCallFunctionPreservesCallerCancellationForVectorFunction` (and its
scalar counterpart), which is the regression test for this race:
- main: 15 failures in 20,000 runs under `-race`;
- this PR: 0 failures in 20,000 runs under `-race`.
`go test ./arrow/compute/...` and `go test -race ./arrow/compute` pass;
`go vet` (with `-asmdecl=false -framepointer=false` for the existing
assembly warnings) and `gofmt` are clean.
### Are there any user-facing changes?
Yes: a cancelled compute call no longer occasionally returns a result
with a nil error. No API changes.
---
arrow/compute/exec.go | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/arrow/compute/exec.go b/arrow/compute/exec.go
index 5a68f11d..528a0409 100644
--- a/arrow/compute/exec.go
+++ b/arrow/compute/exec.go
@@ -166,6 +166,11 @@ func execInternal(ctx context.Context, fn Function, opts
FunctionOptions, passed
ectx := GetExecCtx(ctx)
+ // The kernel watches the caller's context (kctx above), and cancelling
a
+ // parent closes its Done channel before its children are cancelled.
Check
+ // callerCtx as well as ctx below, so a result produced in that gap is
not
+ // returned from a cancelled call.
+ callerCtx := ctx
ctx, cancel := context.WithCancel(ctx)
defer cancel()
@@ -178,14 +183,18 @@ func execInternal(ctx context.Context, fn Function, opts
FunctionOptions, passed
}()
result = executor.WrapResults(ctx, ch, haveChunkedArray(input.Values))
- if ctx.Err() != nil {
+ if callerCtx.Err() != nil || ctx.Err() != nil {
for value := range ch {
if value != nil {
value.Release()
}
}
if err == nil {
- err = context.Cause(ctx)
+ if callerCtx.Err() != nil {
+ err = context.Cause(callerCtx)
+ } else {
+ err = context.Cause(ctx)
+ }
}
if result != nil {
result.Release()
