DanielLeens commented on PR #11931: URL: https://github.com/apache/seatunnel/pull/11931#issuecomment-5391637128
Thanks @SEZ9, this is a sharper and more complete pass than my own on the async/timer/locking themes. I re-checked the current head (`8bc6037b`) directly against your findings rather than taking them at face value: **Issue 1 (identity-guarded `cancellationFutures.remove`)** — verified, and this is a genuinely sharper point than what I raised as my own Issue 1 (which was only about the async-function/timer-flush futures being orphaned, not the `cancellationFutures` removal itself). I confirmed at the current head that `cancellationFutures.remove(taskGroupLocation)` inside `finishOwnedResources()` is still location-keyed, unlike the identity-guarded `executionContexts.remove(taskGroupLocation, ownedContext)` a few lines above it in the same method. To be precise about severity: I don't think this is a live bug today — it only executes on the "owned" branch after `finishExecutionContext()` has already confirmed this tracker still owns the entry, and my own review independently verified the single-caller/single-lock invariant that makes that safe right now. So I'd frame it the same way as your Issue 3: correct today, but correct by convention rather than by construction, on a public method ( `deployLocalTask`) that doesn't enforce the invariant it depends on. Given `finishExecutionContext`'s identity-guarded pattern already exists two lines above in this exact file, applying it to `cancellationFutures` too is cheap enough that I'd support folding it into the blocking list rather than leaving it as a hardening note. **Issues 5-7 (classloader release / future cancellation running inside the full `synchronized (TaskExecutionService.this)` block)** — this is the same locking-scope concern I raised as my own Issue 2, just argued in more detail (the inline-callback-deadlock angle in particular is a good addition I hadn't spelled out). No disagreement, and no new evidence needed on my end — we're aligned. **Issue 2 (stale-path leak of async-function/timer-flush futures, and missing `finishedExecutionContexts` entry for the stale context)** — same territory as my own Issue 1. Agreed on both parts, including the `finishedExecutionContexts` gap, which I hadn't called out as its own point. **Issue 4 (unguarded `recycleClassLoader` can abort `taskDone` before `future.complete` runs)** — this one is new relative to both our prior lists. I checked the current code: `recycleClassLoader(taskGroupLocation, ownedContext)` on both the owned and stale branches is indeed not wrapped in try/catch, while the neighboring `cancelAsyncFunction`/`cancelTimerFlushForTaskGroup` calls are. Confirmed real — if `classLoaderService.releaseClassLoader` throws, the result future never completes and the coordinator hangs waiting on it, which is exactly the failure mode #11679/#11755 are about in the first place. Good catch, +1. **Issue 8 (test doesn't assert the stale-path classloader release)** — same gap I flagged as my own Issue 3, just phrased more precisely around what's missing (`assertNull`/non-empty jars on `oldContext` post-`taskDone`). Net: combining both reviews, I'd move Issue 1 (cancellationFutures identity guard) and Issue 4 (unguarded recycleClassLoader) into the blocking list alongside my own conclusion, given how cheap both fixes are and how directly they sit in the exact method this PR is rewriting for correctness. The rest (2/3/5-8, mine and yours) stay as strong non-blocking recommendations for a fast follow-up. @davidzollo — happy to do a fresh pass on just the delta once a fix commit lands rather than repeating the full template. -- 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]
