DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5459113404
Thanks @SEZ9, and apologies to @abdessalems for the whiplash — I need to correct my own APPROVED review from earlier today. I re-verified both of your blocking findings directly against the current head (`b69dc11615f0`), not just the review text, and both hold up: **Issue 2 (BlockingWorker still reads the shared map, not `ownedContext`)** — confirmed at `TaskExecutionService.java:1086`: ```java TaskGroupContext taskGroupContext = executionContexts.get(taskGroupLocation); ``` This PR's whole point this round was to make context resolution ownership-aware (`TaskGroupExecutionTracker.ownedContext`, introduced at `TaskExecutionService.java:1364`), but the one place that actually needs the race-free lookup — `BlockingWorker.run()` — never switched to it. It still reads through the mutable shared map, so a `deployLocalTask()` `put()` for a newer generation landing between this `get()` and the class-loader dereference at line 1094 hands the old task a new generation's context. The null-guards added in this PR pass in that scenario; they just don't guard against the case they were meant to close. **Issue 1 (`finishOwnedResources()`'s monitor claim is one-sided)** — confirmed. The Javadoc at `TaskExecutionService.java:1494` says the teardown runs "under the same monitor used by deployment," and `finishOwnedResources()` does wrap its body in `synchronized (TaskExecutionService.this)` (line 1503). But `deployLocalTask()`'s installs at lines 592-593 — ```java executionContexts.put(taskGroup.getTaskGroupLocation(), context); cancellationFutures.put(taskGroup.getTaskGroupLocation(), cancellationFuture); ``` — take no lock at all. So the only actual protection is the atomic `executionContexts.remove(key, ownedContext)` inside `finishExecutionContext()` (line 1540) — and that protection is scoped to `executionContexts` alone. `cancellationFutures.remove(taskGroupLocation)` at line 1517 is unconditional and not identity-checked, so a stale tracker that loses the `executionContexts` identity check can still fall through to the "owned" branch's sibling call and delete a newer generation's cancellation future. That's the same class of bug as #11679, just moved one map over. I missed both of these in my own from-scratch pass this morning — I traced the happy path and the specific NPE-to-hang chain from #11679 correctly, but I didn't push on whether the new `ownedContext` guarantee was actually threaded through every reader/writer, which is exactly where it's still open. Treat my APPROVED review as superseded on these two points; I concur with your CHANGES_REQUESTED and both should be blockers before merge. On the rest of your list: agree Issues 3-6 are real (3 and 6 follow directly from the same one-sided-monitor gap; 4 and 5 are consequences of the unsynchronized `deployLocalTask()` install once fixed for 1/3 they mostly collapse together). Issue 7/8 read as reasonable low-severity follow-ups, not blockers. One process note for @abdessalems and reviewers here: #11727's current head already carries an identity-checked `executionContexts.remove(location, ownedContext)` (via `finishExecutionContext()`), which is materially the same fix #11757 (by @waterWang, still open) proposes independently for the same removal-vs-install race. Worth the two of you coordinating so the identity-checked-removal fix lands once, in whichever PR gets there first, rather than in parallel — and #11757's PR description should probably be revisited once the gaps above (cancellationFutures, BlockingWorker's own read path) are closed here, since fixing only the `executionContexts` map leaves the same race open on the sibling maps. Separately, #11910 (the CI-refresh duplicate of this PR) is already closed, so no conflict there. Per this round's process: no new commit has landed since my APPROVED review, so I'm not opening a new formal review right now — this comment stands in its place until a fix lands, at which point I'll do a full from-scratch re-review of the new head. -- 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]
