dongjoon-hyun commented on code in PR #59663:
URL: https://github.com/apache/spark/pull/59663#discussion_r4339138498
##########
core/src/main/scala/org/apache/spark/TaskContextImpl.scala:
##########
@@ -357,27 +521,17 @@ private[spark] class TaskContextImpl(
listenerInvocationThread = Some(Thread.currentThread())
Review Comment:
**3. [Low] The release `finally` starts only after the claim, and no test
covers it**
This claims the invocation, but the `try` whose `finally` releases it starts
only at L609, after `new ArrayBuffer` at L525. Likewise, L278 registers the
waiter before `awaitListenerInvocationHandover` boxes `warned` (L360) for the
`holder.foreach` closure, ahead of its `try` at L362. An `OutOfMemoryError` in
either gap leaves a stale holder or waiter. For example, a background thread
fails the task through `Utils.tryWithSafeFinallyAndFailureCallbacks`, which
only adds such an error as suppressed (`Utils.scala:1336-1338`). That thread
never releases, so the task thread's `markTaskCompleted` now waits forever,
where master only skipped the listeners. My [round-4
comment](https://github.com/apache/spark/pull/59332#discussion_r4256073419) on
#59332 asked for the release to always run, and this is what is left of it.
Separately, removing the release in either `finally` (L615-622, or L399-410)
fails no test, although the description says "Each behavior was checked by
reverting it alone and confirming that a test fails." `runListener` and
`runLeftListeners` catch every `Throwable`, so only a failing `logError` or
`logWarning` gets there, and `LogAppender` ignores its own exceptions.
Could L524-525 move above the claim, and `warned` be updated without a
closure (e.g. with a `match`)? And could a test attach an appender with
`ignoreExceptions = false` that throws on the first "Error in
TaskCompletionListener", and check that a listener added afterwards still runs?
--
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]
