DanielLeens commented on PR #11727:
URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5248822096
@nzw921rx Thanks for taking a close look — glad to have another set of eyes
on this.
**On the ordering concern:** I traced this specifically and I don't think
there's a race here, for a simple reason: `startSignalled` is a plain local
variable declared inside `BlockingWorker.run()` (`boolean startSignalled =
false;`), not a field on the worker or on any shared object.
`submitBlockingTask()` creates one *fresh* `BlockingWorker` instance per task
and submits each exactly once to the executor, so `run()` executes precisely
once per instance, on a single thread, and nothing outside that one method
activation ever reads or writes `startSignalled`. The two touches you flagged —
`startedLatch.countDown(); startSignalled = true;` on the normal path, and `if
(!startSignalled) { startedLatch.countDown(); }` in `finally` — are just two
sequential statements in the same stack frame on the same thread, with a plain
assignment in between them that can't throw. So there's no interleaving window
for another thread to observe `startSignalled == false` after the countdown has
a
ctually happened, and no JMM/visibility concern either, since the variable is
thread-confined by construction. I don't see a double-release or missed-release
path here.
**On reusing `result`/a lifecycle state instead of the boolean:** I like the
instinct, but I don't think `result` can carry this signal without changing
behavior. `result` (the `ProgressState` from `t.call()`) stays `null` until
*after* `t.init()` and the first `call()` complete, whereas the latch is
deliberately released *before* `init()` runs (see the comment right above the
countdown) — that's the whole point of this fix: the deployer must be released
once the worker has resolved its context and is about to start, not once the
task has produced its first result. `ProgressState` also has no "started"
concept today (only `isDone()`/`isMadeProgress()`), so `result.isStarted()`
would require adding new state to that enum. Tying the release to `result`
would push it past `init()`, which risks reintroducing a hang if `init()`
itself blocks — exactly the class of bug #11679 is about. So I think a
dedicated flag is the right shape here; it isn't really standing in for a
reusable ta
sk-lifecycle state, it's tracking something narrower ("has this specific
worker already released its slot of the latch"), which existing state objects
don't model.
**On the name:** agreed that `startSignalled` reads as "has the task
started" when it really means "has this worker already counted down its latch
slot." `startLatchReleased` (or similar) would be clearer. That's a good,
low-risk follow-up — happy to see it done as a quick rename here or in a
fast-follow, purely cosmetic, no behavior change either way.
Net: I don't consider this a blocker for merge, but the naming clarification
is a nice-to-have I'd support taking in this PR if it's an easy touch-up.
--
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]
