You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/09/30 10:48:18 UTC
[PR] [CORE] Run task resource release outside the global registry lock [gluten]
LuciferYang opened a new pull request, #13166:
URL: https://github.com/apache/gluten/pull/13166
## What changes are proposed in this pull request?
The task-completion listener in `TaskResources` held the JVM-global `RESOURCE_REGISTRIES` monitor across `TaskResourceRegistry.releaseAll()` and the peak-memory metric update. On a real executor those release callbacks include native (JNI) teardown that can take milliseconds, so while one finishing task runs them, every other task's completion and registration blocks on the same global monitor.
This moves `releaseAll()` and the metric update outside that lock. The global monitor now only guards the map: it is taken briefly to fetch the registry, and again at the end to remove the entry (guarded so it removes only the registry it actually released). Each `TaskResourceRegistry` already has its own internal lock, so the per-task release work does not need the global one.
Removal still runs in a `finally`, so the entry is dropped even if `releaseAll()` or the metric update throws, and the metric update stays best-effort so it cannot mask a release failure.
## How was this patch tested?
Added `TaskResourceSuite."Run unsafe - release callbacks run outside the global registry lock"`, which starts a second task from inside a release callback and asserts it is not blocked by that callback. It fails if the release pass runs under the global lock.
## Was this patch authored or co-authored using generative AI tooling?
Yes. Generated-by: Claude Opus.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #13166:
URL: https://github.com/apache/gluten/pull/13166#issuecomment-5981859759
Thanks for the test, it reproduces the gap exactly. Applied your change in 0c183192d: `releaseAll()` marks the registry released so a late registration fails with `IllegalStateException`, and the usage recorder is taken before the release so the peak-memory metric still reads it. Your test is in `TaskResourceSuite` as "Run unsafe - registering into a released registry fails", and I updated the description to cover it.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13166:
URL: https://github.com/apache/gluten/pull/13166#issuecomment-6160624960
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13166:
URL: https://github.com/apache/gluten/pull/13166#issuecomment-5981854707
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13166:
URL: https://github.com/apache/gluten/pull/13166#issuecomment-5909670646
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #13166:
URL: https://github.com/apache/gluten/pull/13166#discussion_r4240570620
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -339,6 +358,7 @@ class TaskResourceRegistry extends Logging {
}
priorityToResourcesMapping.clear()
resources.clear()
+ released = true
Review Comment:
The description says every later call on a released registry fails with `IllegalStateException`, but `released = true` is only reached on the normal/`NonFatal` path — a fatal or control-flow throwable from a release callback unwinds past it while the listener's `finally` still removes the map entry, so a thread sharing the `TaskContext` would see `released == false` and could still register into the just-cleared registry (nothing then releases it). Could we set `released` in a `finally` so the guarantee holds on every exit path?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch merged PR #13166:
URL: https://github.com/apache/gluten/pull/13166
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13166:
URL: https://github.com/apache/gluten/pull/13166#discussion_r4266249593
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -339,6 +358,7 @@ class TaskResourceRegistry extends Logging {
}
priorityToResourcesMapping.clear()
resources.clear()
+ released = true
Review Comment:
Fixed in a1fa79219: `releaseAll()` now sets `released` in a `finally`, so the registry is marked released even when a release callback throws a fatal error. The new test "registering after a fatal release failure fails" throws a `LinkageError` from a release callback and checks that the late registration is rejected.
##########
gluten-core/src/test/scala/org/apache/gluten/task/TaskResourceSuite.scala:
##########
@@ -119,4 +122,95 @@ class TaskResourceSuite extends AnyFunSuite with SQLHelper {
// The registry was cleared on the way out, so a fresh task runs cleanly.
TaskResources.runUnsafe(assert(TaskResources.inSparkTask()))
}
+
+ test("Run unsafe - release callbacks run outside the global registry lock") {
+ // On a real executor the release callbacks include JNI native teardown that
+ // can take milliseconds. Running them under the JVM-global registry lock
+ // serialized every concurrent task's completion and registration on the
+ // slowest callback; a concurrent task start must not block on it.
+ var concurrentTaskStarted = false
+ // Captured on the starter thread; read on this thread only after the join
+ // below confirms the thread terminated (the !isAlive assert gates it), which
+ // establishes the happens-before to read these plain locals safely.
+ var starterFailure: Option[Throwable] = None
+ TaskResources.runUnsafe {
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = {
+ val starter = new Thread(
+ () => {
+ try {
+ TaskResources.runUnsafe {
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = {}
+ override def resourceName(): String = "concurrent task resource"
+ }
+ )
+ }
+ concurrentTaskStarted = true
+ } catch {
+ case NonFatal(t) => starterFailure = Some(t)
+ }
+ })
+ starter.start()
+ starter.join(30000)
+ assert(
+ !starter.isAlive,
+ "concurrent task start is still blocked after the release callback returned")
+ }
+ override def resourceName(): String = "slow releaser"
+ }
+ )
+ }
+ // Surface a genuine unrelated failure with its real stack instead of the
+ // misleading "blocked" message below.
+ starterFailure.foreach(t => throw t)
+ assert(concurrentTaskStarted, "concurrent task start was blocked by the release pass")
+ }
+
+ test("Run unsafe - registering into a released registry fails") {
+ // A thread sharing the task's TaskContext can fetch the registry while the release
+ // pass runs outside the global lock. Its registration must fail loudly instead of
+ // landing in the cleared registry, where nothing would ever release it.
+ var lateFailure: Option[Throwable] = None
+ var lateReleased = false
+ var child: Thread = null
+ TaskResources.runUnsafe {
+ val tc = TaskContext.get()
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = {
+ child = new Thread(
+ () => {
+ SparkTaskUtil.setTaskContext(tc)
+ try {
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = lateReleased = true
+ override def resourceName(): String = "late resource"
+ })
+ } catch {
+ case NonFatal(t) => lateFailure = Some(t)
+ } finally {
+ SparkTaskUtil.unsetTaskContext()
+ }
+ })
+ child.start()
+ // Simulate a slow native teardown.
+ Thread.sleep(500)
Review Comment:
Addressed in a1fa79219. The test no longer relies on a fixed sleep. It waits until the registering thread is blocked on the registry's monitor, and it checks for the "already released" message, so the `IllegalStateException` from a missing map entry can't satisfy it.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Run task resource release outside the global registry lock [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13166:
URL: https://github.com/apache/gluten/pull/13166#discussion_r4178357321
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -339,6 +358,7 @@ class TaskResourceRegistry extends Logging {
}
priorityToResourcesMapping.clear()
resources.clear()
+ released = true
Review Comment:
`released` is set only after every callback finishes, but `releaseAll()` deliberately lets fatal and control-flow throwables escape immediately. The completion listener still removes the map entry in its `finally`, so a caller that fetched this registry before removal can acquire its lock after such a failure and successfully register a resource into an unreachable registry. Mark the registry released on every exit (for example, before invoking callbacks or in an unconditional `finally`).
##########
gluten-core/src/test/scala/org/apache/gluten/task/TaskResourceSuite.scala:
##########
@@ -119,4 +122,95 @@ class TaskResourceSuite extends AnyFunSuite with SQLHelper {
// The registry was cleared on the way out, so a fresh task runs cleanly.
TaskResources.runUnsafe(assert(TaskResources.inSparkTask()))
}
+
+ test("Run unsafe - release callbacks run outside the global registry lock") {
+ // On a real executor the release callbacks include JNI native teardown that
+ // can take milliseconds. Running them under the JVM-global registry lock
+ // serialized every concurrent task's completion and registration on the
+ // slowest callback; a concurrent task start must not block on it.
+ var concurrentTaskStarted = false
+ // Captured on the starter thread; read on this thread only after the join
+ // below confirms the thread terminated (the !isAlive assert gates it), which
+ // establishes the happens-before to read these plain locals safely.
+ var starterFailure: Option[Throwable] = None
+ TaskResources.runUnsafe {
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = {
+ val starter = new Thread(
+ () => {
+ try {
+ TaskResources.runUnsafe {
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = {}
+ override def resourceName(): String = "concurrent task resource"
+ }
+ )
+ }
+ concurrentTaskStarted = true
+ } catch {
+ case NonFatal(t) => starterFailure = Some(t)
+ }
+ })
+ starter.start()
+ starter.join(30000)
+ assert(
+ !starter.isAlive,
+ "concurrent task start is still blocked after the release callback returned")
+ }
+ override def resourceName(): String = "slow releaser"
+ }
+ )
+ }
+ // Surface a genuine unrelated failure with its real stack instead of the
+ // misleading "blocked" message below.
+ starterFailure.foreach(t => throw t)
+ assert(concurrentTaskStarted, "concurrent task start was blocked by the release pass")
+ }
+
+ test("Run unsafe - registering into a released registry fails") {
+ // A thread sharing the task's TaskContext can fetch the registry while the release
+ // pass runs outside the global lock. Its registration must fail loudly instead of
+ // landing in the cleared registry, where nothing would ever release it.
+ var lateFailure: Option[Throwable] = None
+ var lateReleased = false
+ var child: Thread = null
+ TaskResources.runUnsafe {
+ val tc = TaskContext.get()
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = {
+ child = new Thread(
+ () => {
+ SparkTaskUtil.setTaskContext(tc)
+ try {
+ TaskResources.addResource(
+ UUID.randomUUID().toString,
+ new TaskResource {
+ override def release(): Unit = lateReleased = true
+ override def resourceName(): String = "late resource"
+ })
+ } catch {
+ case NonFatal(t) => lateFailure = Some(t)
+ } finally {
+ SparkTaskUtil.unsetTaskContext()
+ }
+ })
+ child.start()
+ // Simulate a slow native teardown.
+ Thread.sleep(500)
Review Comment:
The sleep does not establish that the child fetched the registry before the completion listener removes it. If the child is not scheduled within 500 ms, `getTaskResourceRegistry()` throws the same `IllegalStateException` for a missing map entry, so all assertions pass even if the new `released` guard is absent. Use deterministic coordination or test `TaskResourceRegistry` directly so registration is known to be waiting on the registry lock before release completes.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org