DanielLeens commented on PR #11910:
URL: https://github.com/apache/seatunnel/pull/11910#issuecomment-5379408830
# What Problem Does This PR Solve?
- User pain point: same as #11727 — `BlockingWorker.run()` used to resolve
its execution context before entering its `try` block, so a stale-generation
`taskDone()` racing the deploy of a new generation could throw an uncaught NPE
before `startedLatch.countDown()` ever fired, leaving `submitBlockingTask()`
(and the caller holding the `SubPlan` monitor) blocked forever — the silent
hang behind #11679.
- Fix approach: byte-for-byte identical to #11727's current head
(independently re-verified below) — move context/class-loader resolution inside
`try`, throw a descriptive `IllegalStateException` on a missing context,
guarantee `startedLatch.countDown()` fires exactly once via a
`startLatchReleased` flag in `finally`, and add null guards in
`recycleClassLoader()` and `taskDone()`'s handoff to
`finishedExecutionContexts`.
- One-sentence summary: this branch remains a writable-fork CI-refresh
vehicle for #11727, not an independent fix, and nothing about that situation
has changed since my last review.
# 0. Third-round disclosure — this is unchanged since my last review
I've now reviewed this exact content four times total (twice on #11727,
twice on #11910). Live re-verification at the time of this pass:
- **No new commit** on #11910 since my last review (`ffe51c08a551`,
submitted `2026-08-21T14:06:30Z`) — still the current head.
- **No new comments/replies** on #11910 since then.
- I re-diffed `refs/pr-review/11727` (head `72268def15f0`) against
`refs/pr-review/11910` (head `ffe51c08a551`) on the two files that matter —
`TaskExecutionService.java` and `TaskDeployStaleContextRaceTest.java` — and
confirmed **zero diff output**, i.e. still byte-identical. The two PRs continue
to diverge only in unrelated `dev`-drift files that neither PR's own commits
touch.
- I re-read the full current source at the three locations @SEZ9 and I
flagged last round and confirm all three findings are still present and
unchanged (details in 1.1 below) — this is not new analysis, it's independent
re-confirmation that nothing drifted.
- **What has NOT happened since my last review, and should have**: I said in
my prior review that @SEZ9's findings "apply verbatim to #11727's current
head... and should be raised/resolved there." I checked #11727's comment
history live just now — that hand-off never happened. I'm closing that loop
myself in this pass rather than flagging it a third time without acting (see
Section 5).
# 1. Code Change Review
## 1.1 Core Logic Analysis — re-confirmed against current source, not just
carried forward
I independently re-read (not re-quoted) the three locations in question this
pass:
**`recycleClassLoader()`/`taskDone()` ordering
(TaskExecutionService.java:1421-1497) — confirmed real.**
```java
void taskDone(Task task) {
...
if (completionLatch.decrementAndGet() == 0) {
recycleClassLoader(taskGroupLocation);
// re-resolves via get()
TaskGroupContext finishedContext =
executionContexts.remove(taskGroupLocation);
...
}
}
private void recycleClassLoader(TaskGroupLocation taskGroupLocation) {
TaskGroupContext context = executionContexts.get(taskGroupLocation);
// independent lookup
if (context == null) { ...; return; }
context.setClassLoaders(null);
for (Collection<URL> jars : context.getJars().values()) {
classLoaderService.releaseClassLoader(taskGroupLocation.getJobId(),
jars);
}
}
```
`recycleClassLoader()` re-resolves the context via its own
`executionContexts.get()` instead of operating on a reference `taskDone()` pins
down, and it runs *before* `executionContexts.remove()`. Because
`TaskGroupLocation` is reused verbatim across restore generations (the exact
premise of this whole fix), two different generations' trackers can each
observe a non-null context here and each call
`classLoaderService.releaseClassLoader(...)` — a double release at best, and at
worst (if a newer generation has already `put()` its own context under the same
key by the time an older generation's `taskDone()` runs) the *old* generation's
`recycleClassLoader()` nulls out and releases the *new*, still-in-use
generation's class loaders. This is a genuine correctness gap in the exact race
this PR is supposed to be closing, not a hypothetical.
**`getClassLoaders()` NPE window (TaskExecutionService.java:1082-1096) —
confirmed real.**
```java
TaskGroupContext taskGroupContext = executionContexts.get(taskGroupLocation);
if (taskGroupContext == null) {
throw new IllegalStateException(...);
}
Thread.currentThread().setContextClassLoader(taskGroupContext.getClassLoaders().get(t.getTaskID()));
```
Between the null check and the dereference, a concurrent stale
`recycleClassLoader()` can call `context.setClassLoaders(null)` on this exact
same `TaskGroupContext` instance (the ordering bug above makes this more
likely, not less), producing an undiagnosed NPE instead of the deliberate
`IllegalStateException` this PR was written to introduce. Also confirmed:
`.get(t.getTaskID())` returning `null` lets `setContextClassLoader(null)`
succeed silently, so the task would run under the system class loader and fail
much later with a confusing `ClassNotFoundException` rather than failing fast
here.
**`Task.close()` reachable pre-`init()`
(TaskExecutionService.java:1088,1125) — confirmed real, Low severity.**
When the new `IllegalStateException` throws, `result` stays `null`, so the
unchanged `finally` block's `if (result == null || !result.isDone()) {
tracker.task.close(); }` still runs `close()` on a task whose `init()` never
executed and whose task class loader was never installed. Low risk in practice
— `taskGroupExecutionTracker.taskDone(t)` has already run by that point, so the
original hang is already avoided regardless of what `close()` does — but a
connector `close()` that assumes `init()`-populated state could throw a
confusing secondary exception here.
I am not re-deriving @SEZ9's remaining Issues 4/6/7/8 (test
fidelity/busy-spin, reflection-vs-accessor, list-construction nit) a third time
— they were plausible and internally consistent when I checked them last round,
I have no counter-evidence, and re-deriving them again from scratch this pass
would not change their disposition.
## 1.2 Compatibility Impact — Fully compatible (unchanged)
Same-process, in-memory control-flow fix; no
wire/state/checkpoint/serialization changes.
## 1.3 Performance / Side-Effect Analysis (unchanged, restated)
Negligible direct cost. The still-open ordering bug in 1.1 is itself a
side-effect risk (premature/duplicate class-loader release), not a new
observation this round but worth restating since it's the crux of why this
can't be called "ready to merge" as-is.
## 1.4 Error Handling and Logging
| # | Location | Problem | Risk | Suggested fix | Severity | Raised by
another reviewer |
|---|----------|---------|------|----------------|----------|------|
| 1 | `TaskExecutionService.java:1421` (call site), `:1481` (re-`get()`) |
`recycleClassLoader()` re-resolves via `executionContexts.get()` instead of the
caller-pinned instance `taskDone()` intends to recycle | Cross-generation
double-release, or release of a live generation's class loaders | Have
`taskDone()` capture `executionContexts.remove(taskGroupLocation)` first (CHM
guarantees a single winner) and recycle only that specific non-null instance |
Medium | Yes (@SEZ9), independently re-confirmed twice now (my prior review +
this one) |
| 2 | `TaskExecutionService.java:1096` (dereference), `:1493` (concurrent
null-out) | `getClassLoaders()` can NPE between the null-context check and its
use, racing a concurrent `setClassLoaders(null)` | Undiagnosed NPE instead of
the intended `IllegalStateException`; silent null-TCCL fallback if the per-task
lookup misses | Snapshot both the class-loader map and the per-task class
loader into locals; throw the same descriptive `IllegalStateException` when
either is null | Medium | Yes (@SEZ9), independently re-confirmed twice now |
| 3 | `TaskExecutionService.java:1088`, `:1125` | New no-context failure
path still reaches `Task.close()` on a task that never `init()`'ed | Secondary,
confusing exception from a `close()` that assumes `init()`-populated state |
Track an `initialized` flag alongside `startLatchReleased`; skip `close()` on
the pre-init failure path | Low | Yes (@SEZ9), independently re-confirmed |
| 4-8 | See @SEZ9's review body on this PR | Recycle-skip class-loader leak;
test fidelity/busy-spin; reflection vs. accessor; list-construction nit | As
described there | As described there | Medium/Low (per @SEZ9) | Yes (@SEZ9),
not independently re-derived a third time this pass |
# 2. Code Quality Assessment
## 2.1 Coding Standards — unchanged, good (Javadoc, descriptive exception,
ASF header present).
## 2.2 Test Coverage and Test Stability
`TaskDeployStaleContextRaceTest` remains a well-shaped, timeout-bounded,
non-`Thread.sleep` regression test — **Stable**, unchanged from prior rounds.
@SEZ9's Issue 4 (the remover thread's bare `remove()` doesn't simulate the real
`recycleClassLoader()`-then-`remove()` interleaving) is a legitimate
coverage-completeness gap on top of "stable," not a stability regression — it's
precisely why the ordering bug in Issue 1 wasn't caught by CI.
## 2.3 Documentation Updates — none required, unchanged.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution — Precise fix for the bug it targets (latch
never released), with a closely-related bug (recycle ordering) left open in the
same method it touches. Unchanged assessment.
## 3.2 Maintainability — Good, unchanged.
## 3.3 Extensibility — No impact.
## 3.4 Historical-Version Compatibility — No wire/state/checkpoint impact,
unchanged.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| 1 | `recycleClassLoader()` re-resolves via `get()` instead of a
caller-pinned context — cross-generation double-release / live-generation
class-loader release | `TaskExecutionService.java:1421,1481` | Medium |
| 2 | `getClassLoaders()` NPE window between null-check and use |
`TaskExecutionService.java:1096,1493` | Medium |
| 3 | `Task.close()` reachable pre-`init()` via the new failure path |
`TaskExecutionService.java:1088,1125` | Low |
| 4-8 | Recycle-skip leak; test fidelity/busy-spin; reflection vs. accessor;
list nit | see @SEZ9's review | Medium/Low |
| 9 | This PR is a duplicate CI-refresh vehicle for #11727; author has
agreed it should close as superseded, and the findings above still needed
forwarding to #11727 as of this review | N/A (procedural) | High (procedural) |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge on this PR specifically — closing
as superseded, findings forwarded to #11727
This is genuinely unchanged from my last pass, and I don't think re-stating
the same recommendation a third time without acting on it is the right way to
handle that. So, in addition to posting this review, I am taking the two
follow-up actions my last review said should happen but didn't:
1. **Forwarding @SEZ9's Medium findings (Issues 1 and 2 above) to #11727**
as a comment, since the code is byte-identical and #11727 is the PR that will
actually carry the fix forward. These should be resolved there before merge —
they're genuine correctness gaps in the exact class-loader lifecycle this fix
is meant to make safe, caught by @SEZ9 and independently confirmed by me across
two rounds now.
2. **Closing #11910** as superseded by #11727, per @abdessalems's explicit
request on #11727 ("Since #11910 is the duplicate, feel free to close it as
superseded," 2026-08-21T04:09:03Z) — with a closing comment pointing back at
#11727 and this consolidated finding list so nothing raised across these
reviews gets lost.
Nothing on this specific branch needs further code changes — it exists only
as a CI-refresh vehicle and its job (giving the fix a green, in-repo Build run)
is separate from whether the underlying fix is merge-ready, which it is not
yet, pending Issues 1 and 2 above on #11727.
Thanks again to @SEZ9 for the thorough independent pass — the ordering bug
in particular is a real gap that neither #11727's original review rounds nor
mine caught the first time around, and it's exactly the kind of thing this fix
needs closed before it merges.
--
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]
