DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5379413855
Following up here since this is the PR that will actually merge (per the discussion above — thanks @abdessalems for confirming #11910 should close as superseded, which I've now done). Since #11910's head (`ffe51c08a551`) is byte-identical to this PR's current head (`72268def15f0`) on the two files that matter (`TaskExecutionService.java`, `TaskDeployStaleContextRaceTest.java` — re-verified via direct diff just now, zero output), a review posted on #11910 by @SEZ9 applies verbatim here and hadn't been forwarded yet. I independently re-read the current source at each location and confirm both of the following are real, unresolved issues that should be addressed before this merges: **1. `recycleClassLoader()`/`taskDone()` ordering allows a stale generation to release a live generation's class loaders (Medium)** - Location: `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/TaskExecutionService.java:1421` (call site), `:1481` (re-`get()`) - `taskDone()` calls `recycleClassLoader(taskGroupLocation)` *before* `executionContexts.remove(taskGroupLocation)`, and `recycleClassLoader()` independently re-resolves the context via its own `executionContexts.get(taskGroupLocation)` rather than operating on a reference the caller pins down. Because `TaskGroupLocation` is reused verbatim across restore generations — the exact premise this fix is built around — two different generations' trackers can each observe a non-null context here and each call `classLoaderService.releaseClassLoader(...)`. In the worst case, if a newer generation has already installed its own context under the same key by the time an older generation's stale `taskDone()` runs, the old generation's `recycleClassLoader()` nulls out and releases the *new*, still-in-use generation's class loaders — not just a double-release of the same acquisition. - Suggested fix: have `taskDone()` capture `TaskGroupContext finishedContext = executionContexts.remove(taskGroupLocation);` first (the `ConcurrentHashMap` guarantees a single winner), then recycle only that specific non-null instance, instead of `recycleClassLoader()` re-resolving by key. **2. `getClassLoaders()` can NPE between the null-context check and its use (Medium)** - Location: `TaskExecutionService.java:1096` (dereference), `:1493` (concurrent null-out) - `BlockingWorker.run()` null-checks `taskGroupContext` and then dereferences `taskGroupContext.getClassLoaders().get(t.getTaskID())` without a further guard. A concurrent stale `recycleClassLoader()` can call `context.setClassLoaders(null)` on that same instance between the check and the dereference — made more likely, not less, by issue 1 above — producing an undiagnosed NPE instead of this PR's own deliberate `IllegalStateException`. Separately, `.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` instead of failing fast. - Suggested fix: snapshot both the class-loader map and the per-task class loader into locals, and throw the same descriptive "context no longer registered" `IllegalStateException` when either is null. Both are genuine correctness gaps in the exact class-loader lifecycle this fix is meant to make race-safe, not just polish. @SEZ9 also raised several Low-severity items (recycle-skip leaking the current generation's class-loader reference, the regression test's remover-thread fidelity/busy-spin, reflection vs. a package-private test accessor, and a `Collections.singletonList`/`Arrays.asList` nit) — see the full write-up on #11910 for those; I haven't re-derived them independently but have no counter-evidence against any of them. None of this changes my assessment that the overall approach (moving context resolution inside `try`, guaranteeing the latch releases exactly once, guarding the now-more-reachable cleanup path) is the right fix for #11679 — it's a precise fix for the bug it targets. But issues 1 and 2 above are in the same method this PR touches and are part of the same race family, so I'd like to see them closed here before merge rather than filed as a follow-up. -- 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]
