Copilot commented on code in PR #7860:
URL: https://github.com/apache/ignite-3/pull/7860#discussion_r2981483192
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Compute/Compute.cs:
##########
@@ -551,7 +551,7 @@ private async Task<IJobExecution<TResult>>
ExecuteColocatedAsync<TArg, TResult,
},
preferredNode: preferredNode,
expectNotifications: true,
- cancellationToken:
cancellationToken).ConfigureAwait(false);
+ cancellationToken:
CancellationToken.None).ConfigureAwait(false);
Review Comment:
Same cancellation semantics issue as above: switching to
`CancellationToken.None` means a canceled token won’t prevent the colocated
compute request from being sent. Consider checking
`cancellationToken.ThrowIfCancellationRequested()` before issuing the request,
or fix `ClientSocket.RegisterCancellation` to ignore non-cancellable ops so you
can pass the original token without triggering `OPERATION_CANCEL`.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Compute/Compute.cs:
##########
@@ -478,7 +478,7 @@ private async Task<IJobExecution<TResult>>
ExecuteOnNodes<TArg, TResult>(
},
PreferredNode.FromName(GetRandomNode(nodes).Name),
expectNotifications: true,
- cancellationToken: cancellationToken)
+ cancellationToken: CancellationToken.None)
.ConfigureAwait(false);
Review Comment:
Same cancellation semantics issue as above: using `CancellationToken.None`
here prevents early cancellation from stopping the request from being issued.
Either add an explicit `ThrowIfCancellationRequested` before the call, or rely
on the socket-layer cancellation token once it is gated to cancellable ops only.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientSocket.cs:
##########
@@ -739,10 +739,17 @@ private async Task<PooledBuffer> DoOutInOpAsyncInternal(
}
}
- private CancellationTokenRegistration RegisterCancellation(long
requestId, CancellationToken cancellationToken) =>
- cancellationToken == CancellationToken.None
- ? default
- : cancellationToken.Register(() => _ =
CancelRequestAsync(requestId));
+ private CancellationTokenRegistration RegisterCancellation(long
requestId, ClientOp clientOp, CancellationToken cancellationToken)
+ {
+ if (cancellationToken == CancellationToken.None)
+ {
+ return default;
+ }
+
+ Debug.Assert(clientOp.IsCancellable(), "RegisterCancellation for
unexpected op: " + clientOp);
+
+ return cancellationToken.Register(() => _ =
CancelRequestAsync(requestId));
+ }
Review Comment:
`RegisterCancellation` currently registers the callback for any op when a
non-None token is provided; `Debug.Assert(clientOp.IsCancellable())` won’t
prevent redundant `OPERATION_CANCEL` requests in Release builds (and will also
trigger for valid non-cancellable ops that still pass a token, e.g. SQL cursor
paging). Consider guarding with `if (!clientOp.IsCancellable()) return
default;` (optionally keeping the assert/log) so only supported operations
result in `OPERATION_CANCEL` traffic.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Compute/Compute.cs:
##########
@@ -148,7 +148,7 @@ public async Task<ITaskExecution<TResult>>
SubmitMapReduceAsync<TArg, TResult>(
return args.writer;
},
expectNotifications: true,
- cancellationToken: cancellationToken).ConfigureAwait(false);
+ cancellationToken:
CancellationToken.None).ConfigureAwait(false);
Review Comment:
Passing `CancellationToken.None` here means a pre-canceled token will no
longer prevent the compute request from being sent (there are no earlier
`ThrowIfCancellationRequested` checks in this method). This can submit a job
even when the caller already requested cancellation. Prefer either checking
`cancellationToken.ThrowIfCancellationRequested()` before sending, or restoring
the original token once `ClientSocket` only registers server-side cancellation
for ops that actually support `OPERATION_CANCEL`.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Compute/Compute.cs:
##########
@@ -148,7 +148,7 @@ public async Task<ITaskExecution<TResult>>
SubmitMapReduceAsync<TArg, TResult>(
return args.writer;
},
expectNotifications: true,
- cancellationToken: cancellationToken).ConfigureAwait(false);
+ cancellationToken:
CancellationToken.None).ConfigureAwait(false);
Review Comment:
There are compute cancellation tests, but none appear to cover the edge case
where the token is already canceled before `Submit*Async` is called. Since this
change alters how the token is propagated to the socket layer, adding a test
for “pre-canceled token does not submit a compute request / returns promptly”
would help prevent regressions.
```suggestion
cancellationToken: cancellationToken).ConfigureAwait(false);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
