Repository navigation
Make Java SDK context signal-aware - #74690
FrankYang0529 wants to merge 1 commit into
Conversation
Signed-off-by: PoAn Yang <payang@apache.org>
fat-catTW
left a comment
There was a problem hiding this comment.
Thanks for the PR!
lgtm
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the PR, LGTM overall.
| /** Registration of the executing task; resolves wired data inputs. */ | ||
| internal var taskDef: TaskDef? = null | ||
|
|
||
| internal var cancellation = Cancellation() |
There was a problem hiding this comment.
How about constructing the Cancellation() once instead of constructing in several places?
Here're more context that I ask agent to double check:
Only the instance in runTaskAndReport (Server.kt) is connected to the signal trap. The others are defaults: this field, Context.from (line 174), TaskRunner.runTask (Task.kt line 101, never used since both wrappers pass it), and runTask(bundle, request, client) (Task.kt line 170, only TaskTest relies on it).
If a future caller forgets to pass the server's instance, it gets an unconnected one with no error. isCancelled() then marks the wrong instance as watched, and SIGTERM goes to the JVM.
Could we create it once in runTaskAndReport and make the parameter required in Context.from and both runTask functions? Tests can pass Cancellation() explicitly. This field's initializer also allocates an instance that Context.from replaces right away.
| * handles the signal as it would without the SDK. By default the JVM runs its | ||
| * shutdown hooks and exits. | ||
| * | ||
| * A context made with [copy] does not see the stop request. |
There was a problem hiding this comment.
Would it better to block the copy method at all or the share the same Cancellation instance for the Context?
Since is a public data class, copy() is easy to reach for, and the copy gets its own Cancellation that the signal trap never sees. If a task checks only through the copy, the real instance is never marked as watched, so SIGTERM goes to the JVM. Is documenting it enough, or should the copy share the original instance?
Why
[core] killed_task_cleanup_timeseconds, the supervisor sends SIGKILL. The Go and TypeScript SDKs pass that SIGTERM to the task through its context, so the task can stop work it started, such as a job in an external system.How
sun.misc.Signal. It puts the old handlers back after the supervisor has acknowledged the task's result.Context.isCancelled()andContext.awaitCancellation(Duration)tell a task that Airflow asked it to stop.closes: #72680
Was generative AI tooling used to co-author this PR?