DanielLeens commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5601122075
Thanks for the confirmation on `eb7beaa288f` — glad we're converged on the core diagnosis (identity-bound `ownedContext` + shared-monitor critical section closes the TOCTOU race) and on the one real blocker. Point by point, since none of these have changed since my last pass: 1. **F2 (my Issue 1) — stale branch leaks the old generation's own async/timer futures.** Agreed, this is the actual blocker, tracked identically on both our reviews. Nothing to add until a real commit lands. 2. **F1/F4 — monitor scope covering classloader release.** Also unchanged from my assessment: real under `classloader-cache-mode:false` (and I traced the extra `Thread.getAllStackTraces()` cost through `DefaultClassLoaderService.releaseClassLoader()` in my last review for anyone who wants the specifics), but non-blocking under the default config. Worth a follow-up to move the release outside the critical section, not a gate on this PR. 3. **F3 — rollback after the `cancellationFutures.put`/context-publish ordering.** Confirmed pre-existing on `dev` too, not introduced by this diff — filed separately as #12164 so it gets fixed once instead of duplicated across PRs that happen to touch this method. 4. **F6 — unconditional overwrite in `deployLocalTask`.** I re-traced this one independently: `deployTask`'s `containsKey` redeploy guard and `deployLocalTask`'s context-publish both run under the same reentrant `TaskExecutionService.this` monitor, so there's no window for the overwrite to orphan a live future — this is structurally not an issue, not just "probably fine." 5. **F5 — test cleanup.** +1, already flagged as a non-blocking hygiene note; the natural place to fix it is the same new regression test that closes Issue 1/F2, since that test will need to seed both generations anyway. 6. **F7/F8 — documented invariant + warning wording.** Agree these are cheap and worth doing, I just don't rate them as a merge blocker on their own (no incorrect behavior depends on the missing comment, and the log line is a diagnostic aid, not a correctness signal). Given they touch the same method as the Issue 1/F2 fix, the least churn is to fold both into that same commit: a one-line comment on `finishExecutionContext`/`finishOwnedResources` stating that every `executionContexts` writer must hold `TaskExecutionService.this` (matching the existing `@Data`-equals() Javadoc style already in the diff), and rewording the stale-branch log so it says the entry is either absent or owned by a different generation, rather than asserting a newer generation exists. No new commit on this thread yet (head is still `eb7beaa288f`), so there's nothing to re-review right now — once the Issue 1/F2 fix (plus F5/F7/F8 folded in) lands, I'll do a fresh pass on the actual diff. -- 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]
