Copilot commented on code in PR #13107:
URL: https://github.com/apache/gluten/pull/13107#discussion_r4078805458
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -290,12 +294,38 @@ class TaskResourceRegistry extends Logging {
/** Release all managed resources according to priority and reversed order */
private[task] def releaseAll(): Unit = lock {
+ val failures = mutable.ArrayBuffer.empty[Throwable]
priorityToResourcesMapping.toSeq.sortBy(-_._1).foreach {
case (_, resources) =>
- resources.toSeq.reverse.foreach(release)
+ resources.toSeq.reverse.foreach {
+ resource =>
+ // resourceName() is user code too; parse it with a fallback so it
+ // cannot re-abort the release loop.
+ val name =
+ try resource.resourceName()
+ catch {
+ case NonFatal(_) =>
s"resource@${System.identityHashCode(resource)}"
Review Comment:
`resourceName()` is user code, but this fallback catches only `NonFatal`. A
resource that throws an `Error` here (for example, `OutOfMemoryError` or
another linkage error) aborts the loop before lower-priority resources run and
before either registry map is cleared, contrary to the cleanup guarantee.
Handle the same throwable range as the release path, or otherwise put this
lookup behind cleanup that cannot abort the loop.
##########
gluten-core/src/test/scala/org/apache/gluten/task/TaskResourceSuite.scala:
##########
@@ -83,4 +83,33 @@ class TaskResourceSuite extends AnyFunSuite with SQLHelper {
}
assert(unregisteredCount == 2)
}
+
+ test("Run unsafe - one failing release does not skip the remaining
resources") {
+ var goodReleased = 0
+ // Whether the rethrown release failure surfaces from runUnsafe depends on
+ // the Spark version's completion-listener handling, so tolerate both
+ // outcomes and pin only the invariant: the good resource is released
+ // despite the failure.
+ scala.util.Try(TaskResources.runUnsafe {
Review Comment:
Discarding the `Try` result means this test passes even if `releaseAll`
fails to rethrow the release exception, and it never verifies the registry
removal promised by this change. Capture/assert the expected failure and add a
check for the registry-clearing behavior so the test covers the cleanup
contract, not only `goodReleased`.
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -290,12 +294,38 @@ class TaskResourceRegistry extends Logging {
/** Release all managed resources according to priority and reversed order */
private[task] def releaseAll(): Unit = lock {
+ val failures = mutable.ArrayBuffer.empty[Throwable]
priorityToResourcesMapping.toSeq.sortBy(-_._1).foreach {
case (_, resources) =>
- resources.toSeq.reverse.foreach(release)
+ resources.toSeq.reverse.foreach {
+ resource =>
+ // resourceName() is user code too; parse it with a fallback so it
+ // cannot re-abort the release loop.
+ val name =
+ try resource.resourceName()
+ catch {
+ case NonFatal(_) =>
s"resource@${System.identityHashCode(resource)}"
+ }
+ try release(resource)
+ catch {
+ case e: Throwable =>
+ // One failing release must not skip the remaining ones or
leave the
+ // registry uncleared; record the failure and rethrow it after
the
+ // loop so callers still see the error.
+ failures += e
+ logError(s"Failed to release resource $name", e)
+ }
+ }
}
priorityToResourcesMapping.clear()
resources.clear()
+ failures.headOption.foreach {
+ failure =>
+ // Keep the remaining failures attached; the logs are the only other
+ // record and may be swallowed by the completion-listener machinery.
+ failures.tail.foreach(failure.addSuppressed)
Review Comment:
`Throwable.addSuppressed` throws `IllegalArgumentException` when the same
`Throwable` instance is reported by more than one resource. In that case this
cleanup path reports self-suppression instead of rethrowing the first release
failure, so callers do not see the promised original error. Filter out entries
identical to `failure` by reference before adding them as suppressed.
--
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]
