DanielLeens commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5578512508
Thanks for pushing on this, @SEZ9 — glad we're aligned that F2 is the live blocker. Answering your explicit ask so nothing gets re-litigated once the fix lands. **F1/F4 (lock scope):** still exactly as described in my last review, re-verified against `39312b92d`/`eb7beaa28` (no code change). `finishOwnedResources()` (`TaskExecutionService.java:1542-1565`) and `cancelAllTask()` (`:1420-1449`) both run `recycleClassLoader()`/`cancelAsyncFunction()`/`cancelTimerFlushForTaskGroup()` inside `synchronized (TaskExecutionService.this)`, the same monitor `deployLocalTask()` takes to publish a new generation. That's real contention exposure only under the non-default `classloader-cache-mode:false` (per-job isolation adds real work under the lock); under the default `true` it's a cheap no-op path. I'm keeping this as a non-blocking follow-up, not a blocker on this diff — closing it properly means moving the actual cancel/release calls outside the critical section while keeping the ownership check inside it, which is a separate, larger change from the generation-tagging F2 needs. **F3 (`cancellationFutures.put` before `onContextPublished.run()`, no rollback on a later throw):** confirmed still present at `deployLocalTask:671-692`, and confirmed still pre-existing — this exact catch block is byte-identical to `origin/dev`, so this PR's diff doesn't introduce or worsen it. Filed separately as #12164 rather than folding it into this PR's scope; not a blocker here. **F6 (`deployLocalTask` overwrites `cancellationFutures` unconditionally):** traced this one through rather than just repeating my prior read. The overwrite at line 677 can only execute for a `TaskGroupLocation` that has already cleared the `executionContexts.containsKey(...)` guard in `deployTask()` (`:507`) — i.e. the previous generation's context is no longer "active." The only way a context leaves "active" is via `finishExecutionContext()`'s success branch inside `finishOwnedResources()`, which removes the *same* location's `cancellationFutures` entry (`:1553`) under the *same* `synchronized (TaskExecutionService.this)` monitor that `deployTask()`'s publish block also holds. So by construction there's no interleaving where a live cancellation future is still sitting under that key when the new `put` executes: either the old generation is still active (new deploy short-circuits at `:507` before touching `cancellationFutures` at all), or it already went through the success pat h and cleared its own entry first. So it's structurally neutralized today, not just "probably fine" — though it does depend on that invariant staying implicit rather than asserted or commented at line 677, which is fair to tighten alongside the F2 fix if you want an explicit guard there too, but I wouldn't gate merge on it. On the fix shape you outlined for F2 (generation-tag the async/timer maps, cancel in the stale branch alongside `recycleClassLoader`, and a test that deploys gen A/B for the same location and asserts A's futures are cancelled while B's stay live, with cleanup so the injected entries don't leak into later tests) — agreed, that's the right shape and covers F5's cleanup concern in the same test. I'll do a full from-scratch pass on the next commit rather than a delta diff, same as every round so far. -- 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]
