DanielLeens commented on PR #12271: URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-5645053645
Thanks @SEZ9 - I went back and verified both points directly against `9f98c147ce23` (same head you reviewed) rather than taking them on faith, and they hold up. This is a solid extension of the two things I flagged in my own pass, with sharper evidence on both: **Your Issue 1 (recycleClassLoader/taskDone still location-keyed) = my Issue 2, confirmed and sharpened.** I checked `TaskExecutionService.java:1453-1520` myself: - `taskDone()` (1453-1508) calls `recycleClassLoader(taskGroupLocation)` (1461) and then `executionContexts.remove(taskGroupLocation)` (1463) - both keyed purely by the reused `TaskGroupLocation`, never by `ownedContext` identity. - `recycleClassLoader` (1510-1516) does `executionContexts.get(taskGroupLocation).setClassLoaders(null)` - if generation B has already published its context at that location (line ~675 in `deployLocalTask`), a straggling generation A's cleanup nulls B's live loader map and releases B's jars out from under it. - The part I hadn't traced as precisely as you did: `getTaskClassLoader()` (1518-1520) just returns `ownedContext.getClassLoader(taskId)`, and once that map is nulled this returns `null` silently instead of the old `NullPointerException` from the pre-PR `executionContexts.get(location).getClassLoaders().get(...)` chain. That's a real regression in failure signal quality on top of the correctness bug - it used to fail loud, now it fails quiet. Agreed this should be a blocker, not a follow-up. **Your Issue 2 (BlockingWorker still location-keyed) = my Issue 1, and you found something worse than what I flagged.** I only pointed out that the PR/issue description incorrectly claims #11727 already covers `BlockingWorker`, which is unmerged (`gh pr view 11727` still shows `state: OPEN`, `mergedAt: null`) - a documentation/scope-accuracy problem. You went further and showed it's an active deadlock risk, which I confirmed: `BlockingWorker.run()` (`TaskExecutionService.java:1132-1136`) resolves the loader *before* the `try` block that starts at 1141, so if that lookup NPEs (context removed/nulled by a stale generation's `taskDone`), `startedLatch.countDown()` (1142) never runs. I checked `submitBlockingTask` (`TaskExecutionService.java:384-411`): it does `uncheckRun(startedLatch::await)` at line 410 and does not return until every worker's latch counts down - so that NPE doesn't just fail one task, it hangs the deploying thread indefinitely with the task group never reaching a t erminal state. That's strictly worse than "same bug, different path" - it's a hang, not just a misload. Given `Task.isThreadsShare()` defaults to `false` (execution/Task.java:44-46) and nothing overrides it, this is the common path under both `OFF` and `PART`, not an edge case. Net: I'm changing my "blocking on Issue 1 (description fix) + non-blocking on Issue 2 (write-path race)" stance to align with yours - both need code fixes before merge, not just a description correction. Your suggested fixes (identity-checked `executionContexts.remove(location, ownedContext)` in `taskDone`, `ownedContext`-based `recycleClassLoader`, routing `BlockingWorker` through `taskGroupExecutionTracker.getTaskClassLoader(...)` with the lookup moved inside the `try`) match what #11727 already prototypes for the write side, so pulling that pattern into this PR (or landing #11727 first and rebasing this on top) both seem like reasonable paths - your call which is less disruptive given where #11727 currently stands. @zhangshenghang - given both independent reviews converge on the same two gaps (stale-generation cleanup corrupting the live context, and BlockingWorker keeping the original race under the default execution mode with an added hang risk), I'd treat these as must-fix before merge rather than follow-ups. -- 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]
