-
Notifications
You must be signed in to change notification settings - Fork 5k
[Improvement-17699][GrpcTask] Support cancel gRPC task #17700
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
base: dev
Are you sure you want to change the base?
Changes from 1 commit
9351a55
57c5003
9137e02
9b808ab
153676a
38a8c22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -37,6 +37,7 @@ | |||||
| import com.google.protobuf.util.JsonFormat; | ||||||
| import com.google.protobuf.util.JsonFormat.Printer; | ||||||
|
|
||||||
| import io.grpc.Context; | ||||||
| import io.grpc.ManagedChannel; | ||||||
| import io.grpc.Status; | ||||||
| import io.grpc.StatusRuntimeException; | ||||||
|
|
@@ -47,6 +48,7 @@ public class GrpcTask extends AbstractTask { | |||||
|
|
||||||
| private GrpcParameters grpcParameters; | ||||||
| private TaskExecutionContext taskExecutionContext; | ||||||
| private volatile Context.CancellableContext cancellableContext; | ||||||
|
|
||||||
| /** | ||||||
| * constructor | ||||||
|
|
@@ -71,36 +73,87 @@ public void init() { | |||||
|
|
||||||
| @Override | ||||||
| public void handle(TaskCallBack taskCallBack) throws TaskException { | ||||||
| ManagedChannel channel = null; | ||||||
| try { | ||||||
| ManagedChannel channel; | ||||||
| if (grpcParameters.getChannelCredentialType() == GrpcCredentialType.TLS_DEFAULT) { | ||||||
| TlsChannelCredentials creds = (TlsChannelCredentials) TlsChannelCredentials.create(); | ||||||
| channel = GrpcDynamicService.ChannelFactory.createChannel(grpcParameters.getUrl(), creds); | ||||||
| } else { | ||||||
| channel = GrpcDynamicService.ChannelFactory.createChannel(grpcParameters.getUrl()); | ||||||
| } | ||||||
|
|
||||||
| Descriptors.FileDescriptor fileDesc = | ||||||
| JSONDescriptorHelper.fileDescFromJSON(grpcParameters.getGrpcServiceDefinitionJSON()); | ||||||
| GrpcDynamicService stubService = new GrpcDynamicService(channel, fileDesc); | ||||||
| DynamicMessage message = stubService.call(grpcParameters.getMethodName(), grpcParameters.getMessage(), | ||||||
| grpcParameters.getConnectTimeoutMs()); | ||||||
| Printer printer = JsonFormat.printer().omittingInsignificantWhitespace(); | ||||||
| addDefaultOutput(printer.print(message)); | ||||||
| } catch (StatusRuntimeException statusre) { | ||||||
| validateResponse(statusre.getStatus()); | ||||||
| return; | ||||||
| } catch (Exception e) { | ||||||
| throw new GrpcTaskException("gRPC handle exception:", e); | ||||||
|
|
||||||
| // → Attach a cancellable gRPC Context to support external cancellation. | ||||||
| // This context propagates cancellation signals to the underlying RPC call. | ||||||
| this.cancellableContext = (Context.CancellableContext) Context.current().withCancellation().attach(); | ||||||
| Context previous = this.cancellableContext; | ||||||
|
|
||||||
| try { | ||||||
| GrpcDynamicService stubService = new GrpcDynamicService(channel, fileDesc); | ||||||
|
|
||||||
| // → Perform the actual blocking gRPC call. | ||||||
| // If cancel() is invoked concurrently, this call will throw StatusRuntimeException(CANCELLED). | ||||||
| DynamicMessage message = stubService.call( | ||||||
| grpcParameters.getMethodName(), | ||||||
| grpcParameters.getMessage(), | ||||||
| grpcParameters.getConnectTimeoutMs()); | ||||||
|
|
||||||
| // → Format and store the response as task output | ||||||
| Printer printer = JsonFormat.printer().omittingInsignificantWhitespace(); | ||||||
| addDefaultOutput(printer.print(message)); | ||||||
| validateResponse(Status.OK); | ||||||
| } finally { | ||||||
| // → Detach the cancellable context to restore the previous context and avoid leaks | ||||||
| this.cancellableContext.detach(previous); | ||||||
| this.cancellableContext = null; // Clear reference for GC and idempotent cancel | ||||||
| } | ||||||
| } catch (StatusRuntimeException ex) { | ||||||
| validateResponse(ex.getStatus()); | ||||||
| } catch (Exception ex) { | ||||||
| setExitStatusCode(TaskConstants.EXIT_CODE_FAILURE); | ||||||
| throw new GrpcTaskException("gRPC handle exception", ex); | ||||||
| } finally { | ||||||
| // → Gracefully shut down the gRPC channel to release network resources | ||||||
| if (channel != null) { | ||||||
| channel.shutdown(); | ||||||
| } | ||||||
| } | ||||||
| validateResponse(Status.OK); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public void cancel() throws TaskException { | ||||||
| // Do nothing when task to be canceled | ||||||
| public void cancel() { | ||||||
|
||||||
| public void cancel() { | |
| public void cancel() throws TaskException { |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The cancel() method wraps the caught exception in a TaskException and re-throws it, but this may cause issues since cancel() is typically called from a different thread (the cancellation thread). Consider logging the error instead of throwing an exception, as throwing from cancel() may not properly propagate to the caller. Most other task implementations (e.g., ShellTask) throw from cancel, so this follows the pattern, but the additional wrapping might not be necessary.
| throw new TaskException("Cancel gRPC task failed", ex); | |
| // Do not throw from cancel(); just log the error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect context management pattern. The
attach()method returns the previous context that was attached, not the newly created cancellable context. This code incorrectly tries to cast the return value toCancellableContextand then assigns it again toprevious.The correct pattern should be:
This separates the creation of the cancellable context from the attachment, and correctly captures the previous context returned by
attach().