dongjoon-hyun opened a new pull request, #59794: URL: https://github.com/apache/spark/pull/59794
### What changes were proposed in this pull request? This PR makes the logging of a failed pre-completion listener in `TaskContextImpl.invokeTaskPreCompletionListener` best effort. If `logError` throws, the failure is added to the failure of the listener with `addSuppressed`, like the failure of `markTaskFailed(e)` right above it, instead of ending the loop in `markTaskCompleted()`. It is then reported by the `TaskCompletionListenerException` that `markTaskCompleted()` throws once all listeners have run. Like a failure thrown by a listener, an error thrown by `logError` is reported in that exception and not rethrown as is, even if it is fatal. Guarding a log call this way, where a later step must not be skipped, is not new: see `SparkUncaughtExceptionHandler` before it halts the process, `TaskMemoryManager.logPageAllocationFailure`, and the warnings of `PipelinedWriterTask.awaitExit`. The same unguarded `logError` is in `invokeListeners()`, which runs the completion and failure listeners. That method is being reworked in apache/spark#59663, so it is left alone here. That PR does not touch `invokeTaskPreCompletionListener()`. Both PRs add a `FailingAppender` with the same signature to `TaskContextSuite`, so whichever lands second needs a trivial rebase of the test file. ### Why are the changes needed? `markTaskCompleted()` runs the pre-completion listeners in a loop and sets `completed` only when it finds none left. The completion listeners run after that. `invokeTaskPreCompletionListener()` catches the failure of a listener, but then logs it without a guard. If `logError` throws, the loop ends before `completed` is set. This happens when an appender configured with `ignoreExceptions=false` (the default is `true`) fails: log4j 2.26.1 then throws an `AppenderLoggingException` to the caller of the logging method. Any other error thrown by the logging call has the same effect. The task is then stuck for good: - the `markTaskCompleted(None)` that `runTaskWithListeners` calls in `finally` returns at `if (completionStarted) return`, - `addTaskCompletionListener()` only queues a listener while `completed` is false, and - `addTaskPreCompletionListener()` pushes onto a stack that nobody drains. So the remaining pre-completion listeners, e.g. the one that stops a second pipelined Python UDF writer (SPARK-60113, apache/spark#59332), and all completion listeners never run. `isCompleted()` stays false, so `BasePythonRunner.MonitorThread` keeps polling, and the executor frees the memory of the task (`TaskMemoryManager.cleanUpAllAllocatedMemory()`) while a pipelined writer may still use it, which is the use-after-free that SPARK-60113 prevents. Setting `completed` in a `finally` would not do: the completion listeners must not run before the remaining pre-completion listeners, and a pre-completion listener left in the stack when `completed` is set would never run. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added a test to `TaskContextSuite`. It registers two pre-completion listeners, of which the first to run fails, and a completion listener. An appender that does not ignore exceptions fails the logging of the "Error in TaskPreCompletionListener" message. The test checks that `markTaskCompleted()` runs the second pre-completion listener and then the completion listener, completes the task, and reports the failure to log with the failure of the listener. Without the change, the test fails because the `AppenderLoggingException` escapes `markTaskCompleted()` and none of the remaining listeners runs. With the change, it passes. ``` build/sbt 'core/testOnly *TaskContextSuite' build/sbt 'core/testOnly *BasePythonRunnerSuite' ``` ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Sonnet 5.5 -- 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]
