dongjoon-hyun commented on code in PR #59663:
URL: https://github.com/apache/spark/pull/59663#discussion_r4339138469
##########
core/src/main/scala/org/apache/spark/TaskContextImpl.scala:
##########
@@ -217,12 +239,45 @@ private[spark] class TaskContextImpl(
// writer that adds listeners lazily while it pulls its input. And since
the completion
// listeners only run after `completed` is set, they run after every
pre-completion listener,
// even if another thread is invoking listeners at this point.
+ //
+ // The completion listeners then run on this thread too, and this method
returns only after
+ // they have run. If it returned while another thread is invoking
listeners (e.g. a kill thread
+ // running failure listeners), that thread would not run them, so cleanup
would be skipped,
+ // and the task would release its memory and report its result anyway. So
the listener
+ // invocation is claimed together with setting `completed`, which also
keeps a thread that adds
+ // a completion listener at that moment from running them; if another
thread holds it, that
+ // thread hands it over to this one when done. Failure listeners pending
at that point run
+ // first (see `invokeListeners()`). A completion listener added after this
method has run the
+ // others runs on the thread adding it, or, if another thread is invoking
listeners at that
+ // moment, on that thread before it stops (see `invokeListeners()`).
+ //
+ // The wait for the other thread has no time bound: if its listeners
block, so does this task.
+ // It logs a warning periodically, the first one with that thread's stack
trace. A kill thread
+ // runs the failure listeners holding the lock of the executor's
TaskRunner, which the task
Review Comment:
**1. [Medium] The handover wait now comes before the task releases its block
locks and memory, so the kill path can deadlock**
"This only adds a wait for other threads" holds for the `TaskRunner` lock,
but on master the task thread blocks on that lock only after its cleanup. When
a kill thread holds the invocation, master's `markTaskCompleted` returns at
once, `Task.run` releases the unroll memory in its `finally`
(`Task.scala:152-153`), and the `TaskRunner` releases the task's block locks
and memory (`Executor.scala:916-917`). Only then does the task thread block in
`setTaskFinishedAndClearInterruptStatus` (`Executor.scala:746`). With this PR,
it waits at L292-293, before all of that.
So a failure listener on the kill thread that waits for anything released in
that cleanup now deadlocks. For example, a third-party interrupt listener
throws, so `Task.kill` runs the failure listeners on the kill thread while it
holds the `TaskRunner` lock and the JVM-wide `TaskContext` lock
(`Task.scala:235`). One of them calls `blockManager.removeBlock(b)`, whose
`lockForWriting` waits for the read lock that the killed task still holds on
the cached block `b` until `Executor.scala:916`. On master this finishes as
soon as the task thread reaches its cleanup. Now neither thread moves, and
since the kill thread keeps the JVM-wide lock, every later `Task.kill`,
`interruptible()` and `createResourceUninterruptibly` on the executor blocks
too. On a reaper thread, `kill()` never returns, so the JVM kill does not fire
either.
In-tree listeners don't do this (RocksDB's failure listener only aborts, and
the JDBC interrupt listeners catch `NonFatal`), so it takes third-party code.
Still, the description's "On the kill path this is not new" and this comment
say that the kill path is not affected. Could they say instead that the task
now waits before it releases its block locks and memory, so a failure listener
on a kill thread must not wait for either?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
