-
Couldn't load subscription status.
- Fork 181
Fix activity completion when thread interrupted flag is set #2694
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: master
Are you sure you want to change the base?
Conversation
|
Sojan Mathew seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
8544c12 to
69d2363
Compare
Activities that catch InterruptedException, restore the interrupted flag, and continue execution were unable to report their completion due to gRPC call failures. This occurred because gRPC calls fail when the thread's interrupted flag is set. The fix clears the interrupted flag before making gRPC calls in ActivityWorker.sendReply() and restores it afterward, ensuring: - Activity results are successfully reported to the server - Thread interruption semantics are preserved for worker shutdown - All activity completion scenarios work (success/failure/cancellation) Additionally, extract executeGrpcCallWithInterruptHandling() method to eliminate code duplication across the three gRPC response calls, improving maintainability and reducing the risk of inconsistent implementations. Fixes: temporalio#731 Related: temporalio#722
0e83e9b to
39f8cd8
Compare
| */ | ||
| private void executeGrpcCallWithInterruptHandling(Runnable grpcCall) { | ||
| // Check if the current thread is interrupted before making gRPC calls | ||
| // If it is, we need to clear the flag to allow gRPC calls to succeed,then restore it after reporting. |
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.
What part of the SDK relies on it being restored?
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.
What part of the SDK relies on it being restored?
@Quinn-With-Two-Ns
TheCancellationExceptionis thrown in GrpcSyncRetryer when reporting activity completion results:
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new CancellationException();
}
Fix the bug where activities that return with the 'interrupted' flag set, were unable to report their completion to the server. Activities can now successfully report completion even when they have the 'interrupted' flag set.
The fix clears the 'interrupted' flag before making gRPC calls in ActivityWorker.sendReply() and restores it afterward, ensuring:
Fixes: #731
Related: #722
What was changed
Fix the bug where activities that return with the 'interrupted' flag set were unable to report their completion to the server due to gRPC call failures.
Why?
Allow activities to successfully report their results even when they have the 'interrupted' flag set.
The fix ensures that activities that handle interruption gracefully and report their results to the Temporal server, while
maintaining proper thread interruption semantics for the worker shutdown process.
According to the issue description and PR #722 , the issue was that when an activity:
1 Catches an InterruptedException
2 Restores the 'interrupted' flag (Thread.currentThread().interrupt())
3 Continues to run and returns a result
The subsequent gRPC calls in ActivityWorker.sendReply() would fail with CancellationException: The gRPC request was
cancelled because the 'interrupted' flag was still set, preventing the activity result from being reported to the
server.
The CancellationException is thrown in GrpcSyncRetryer when reporting activity completion results:
Solution
Modify ActivityWorker.sendReply() method to:
1 Check the 'interrupted' flag before making gRPC calls using Thread.interrupted() (which also clears the flag)
2 Temporarily clear the flag to allow gRPC calls to succeed
3 Make the gRPC call to report the activity result
4 Restore the 'interrupted' flag in a finally block if it was originally set
@Spikhalskiy
Checklist
Closes Activities that return with interrupted flag should be successfully reported as Completed #731
How was this tested:
1 Created and ran a comprehensive test that reproduces the exact scenario described in the bug report
2 Verified the test failed before the fix (confirming the bug existed)
3 Verified the test passes after the fix (confirming the bug is resolved)
4 Ran existing shutdown tests to ensure no regressions were introduced
Any docs updates needed?
No