DanielLeens commented on PR #11757:
URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5473725599
Thanks @SEZ9 for the thorough pass on `863ddb172a58` — a real second set of
eyes on a critical-section rewrite, and most of it holds up. A few notes to
keep the thread precise.
**No new commit since my last review.** The head is unchanged from what I
reviewed on 2026-08-29T11:45:53Z, so this is a targeted response to your
findings rather than a fresh full re-review.
**Still the primary open blocker, which your review doesn't touch: my own
Issue 1 from that review.** `finishExecutionContext()` on this head lost the
explicit `activeContext != ownedContext` reference-identity pre-check that was
present as of `5a0a5def3dcb` — it now relies solely on
`ConcurrentMap.remove(key, value)`, whose match semantics fall back to
`TaskGroupContext`'s Lombok-generated *structural* `equals()`. That's correct
today only because `TaskGroupDefaultImpl` doesn't override
`equals()`/`hashCode()`; if that ever changes, the CAS can silently match a
different generation's structurally-equal context and reintroduce exactly the
bug this PR exists to fix, with no compiler error and no test to catch it.
Sibling PR #11999 still has the identity check verbatim. I'd keep that as the
High-severity blocker gating merge, ahead of the items below.
+1 to your **Issue 1** (stale branch releases its own classloaders but
leaves its own async-function/timer-flush futures uncancelled) — one added
detail: this is the same gap I logged as Issue 4 in my 2026-08-29 review, which
you yourself first raised on 2026-08-23 (your then-Issue 5). Root cause is
unchanged: `taskAsyncFunctionFuture`/`timerFlushFutures` carry no generation
identity, so a blanket cancel in the stale branch risks cancelling the *newer*
generation's own futures instead — fixing it properly needs the same
ownership-tagging this PR just added for `TaskGroupContext`, which is why we've
both been treating it as a tracked follow-up rather than a blocker on this diff.
Your **Issue 4** (teardown now runs under the shared
`TaskExecutionService.this` monitor) is the same contention concern I restated
in my 1.3/Issue 2 on 2026-08-29 — no new detail to add, just confirming
alignment, non-blocking.
On **Issues 3 and 5** (the `contextPublished`-guarded failure unwind in
`deployLocalTask()` "still removes by location alone") — I don't think this
matches the code on this head, worth correcting before it gets carried into a
follow-up fix. Every reference to `contextPublished` in the file:
```
631: boolean contextPublished = false;
678: contextPublished = true;
689: if (!contextPublished) {
```
The only place it gates anything is the `catch` block at the bottom of
`deployLocalTask()`:
```java
} catch (Throwable t) {
logger.severe(ExceptionUtils.getMessage(t));
if (!contextPublished) {
onFailureBeforeContextPublished.accept(t);
}
resultFuture.completeExceptionally(t);
}
```
There is no `executionContexts.remove(...)` call anywhere in this method,
gated or otherwise — the only two mutators of `executionContexts` in the whole
class are the `put` at line 676 (inside the new `synchronized` block) and the
ownership-checked `remove(key, value)` at line 1552 (`finishExecutionContext`).
I checked `origin/dev` directly too: this catch block is byte-identical to
pre-PR `dev` (this PR's diff doesn't touch it at all), so there's no
key-only-remove rollback here in either version, and this PR doesn't
reintroduce anything at this call site.
What *is* real, and different from what you described: if `contextPublished
== true` and something throws afterward (e.g.
`submitThreadShareTask`/`submitBlockingTask`/`taskGroup.setTasksContext`),
nothing ever cleans up that deployment's own
`executionContexts`/`cancellationFutures` entry — it's an orphan-leak risk on
the failed deployer's own state, not an eviction risk on a newer generation,
and it predates this PR (same gap exists on `dev` today, unmodified by this
diff). Worth a separate follow-up, but I'd drop it as a blocker on this PR
since it isn't something this diff introduced or worsened.
On **Issue 2** — the `cancelTaskGroup` example doesn't quite hold up as a
counter-case: `cancelTaskGroup()` (`TaskExecutionService.java:765`) only does
`cancellationFutures.get(location).cancel(false)`; it never `put`/`remove`s the
map, so it isn't actually a mutator of the invariant you're describing. The
broader point — the paired-map invariant is only enforced at the two sites this
PR touches, and any future unguarded mutator would break it — is fair as an
architectural note, it just needs a real example to be actionable as a blocking
item.
**Issue 6** (exception escaping `finishOwnedResources()` skips
`resultFuture` completion) checks out against the code —
`finishOwnedResources()` is called unguarded at `taskDone():1469`, and
`recycleClassLoader()`'s call to `classLoaderService.releaseClassLoader(...)`
has no try/catch. Worth noting this specific gap predates this PR too — the
pre-PR `recycleClassLoader(taskGroupLocation)` call in `taskDone()` was just as
unguarded — so it's a legitimate, real issue but not a new regression from this
diff.
**On the #11757/#11999 overlap**: as I noted in my 2026-08-29 review, #11999
already carries the identity-check fix that's missing here (verbatim), plus the
recycle-classloader-in-stale-branch fix both PRs now share. I'd treat resolving
the #11757/#11999 duplication as a prerequisite to merging either one — the
dedicated #11999 review in this batch is covering that analysis in depth, so I
won't duplicate it here.
--
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]
