DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5419385522
@abdessalems @davidzollo - sorry for the mixed signals, and thanks for holding off and asking rather than guessing again. I went back through both diffs (not from memory) before answering. I'm changing my recommendation: **David is right, and I'd go further than the "tight" split proposed above.** Why the reversal: the reason this matters more than a normal scope call is that #11727 itself changes when `taskDone()` runs. On `dev` today, the classloader lookup happens *before* the `try/finally`, so a null context throws before `taskGroupExecutionTracker.taskDone(t)` is ever reached - the stale-generation cleanup path in `taskDone()` is unreachable from this failure. #11727 moves that lookup inside the `try`, so the same null-context case now falls through to `finally` and calls `taskDone()` for the first time. That's a real, new trigger for the #11755 race, introduced by this PR's own fix - not a hypothetical shared by both PRs equally. David's "the cleanup path needs to be generation-safe before merge" is correct on the code, not just a preference. One correction to the "tight" version floated above (owned-context + identity-checked remove only, leaving the rest to #11757): that's not sufficient on its own. In the current `taskDone()` body, `cancellationFutures.remove(taskGroupLocation)`, `cancelAsyncFunction(taskGroupLocation)`, and `cancelTimerFlushForTaskGroup(taskGroupLocation)` all run unconditionally, straight after the context/classloader recycle step. If only the context swap is identity-gated, a stale tracker can still drop the newer generation's cancellation future and cancel its async functions and timer-flush tasks - same corruption class, different door. That's exactly why #11757 wraps all five steps (classloader recycle, context handoff, cancellationFutures removal, async-function cancel, timer-flush cancel) inside one identity-checked `synchronized` block (`finishOwnedResources()` / `finishExecutionContext()`), not just the context piece. Concrete recommendation: don't reimplement a narrower subset in #11727. Port the `ownedContext` field and the `finishOwnedResources()`/`finishExecutionContext()` pair from #11757 wholesale - it's already written, already covers the full bundle, and already has its own regression test (`testStaleTaskDoneDoesNotCleanupNewerGenerationResources`). @waterWang, would you be up for either pushing that commit directly onto abdessalems' branch or letting him cherry-pick it with credit, so it isn't written twice? Once that's in #11727, #11757 rebases on top and should shrink to whatever's left beyond the ownership guard, if anything. Keep the `getClassLoaders()` null-guard and the finally-block latch release in #11727 as-is - those are still correct and useful as a defensive backstop even with the ownership check in place. This should be the last open question before this is mergeable; thanks again for the patience through all the rounds. -- 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]
