loustler commented on PR #11718:
URL: https://github.com/apache/seatunnel/pull/11718#issuecomment-5233981707
@DanielLeens both findings check out. Documented in `066c5c66` and in the PR
description, and I went looking for a third thing while I was in there.
## Issue 1 — confirmed, and it is the `runningCount` bookkeeping exactly as
you described
I read `SeaTunnelContainer#doExecuteJob` rather than reasoning from your
summary, and the shape is what you said:
```java
runningCount.incrementAndGet();
Container.ExecResult result = executeJob(server, confFile, jobId,
variables); // blocks here
if (runningCount.decrementAndGet() > 0) {
// only check thread when job all finished.
return result;
}
```
An abandoned worker never reaches the decrement, `PER_CLASS` means the
counter is shared across all four methods, so every later submission sees
non-zero and skips the leak assertion. Deterministic, exactly as you traced it.
Two qualifications I would add, neither of which softens the finding.
It is **not a regression**. Today a hang means the later tests never execute
at all, so the leak check is not running either — this is a gap in the new
"continue after a timeout" path rather than coverage this PR takes away. That
is a reason to document it, not a reason to shrug at it: someone chasing a
stale-thread report on this class after a timeout would otherwise be looking
for a signal that was silently suppressed.
And it **cannot be fixed from this class**. `runningCount` is `private` with
no accessor, so resetting it means changing `SeaTunnelContainer` — the
shared-infrastructure change we both agreed this PR should stay out of. So your
Option B it is, and for the reason you gave rather than because Option A was
hard.
## Issue 2 — you were right, and my explanation was wrong for a different
reason than the conclusion suggested
I checked this against the actual bytecode of `junit-jupiter-api-5.9.0`
rather than my memory of the API, and `assertTimeoutPreemptively` does call
`ExecutorService#shutdownNow()` — two call sites in the private overload, on
both the normal and the exceptional path. So the worker *is* interrupted;
"abandons rather than interrupts" was simply wrong about the mechanism.
The conclusion survives, but on a different basis than I originally wrote:
the worker is blocked in a blocking socket read against the Docker daemon, and
that does not respond to `Thread.interrupt()`. Corrected in the code and the
description, since a comment that is right by accident is worse than no comment.
## A third one, which I would not have gone looking for without your Issue 2
Checking `shutdownNow` put me in `AssertTimeout`, and the thread factory a
few lines down is:
```java
public Thread newThread(Runnable runnable) {
return new Thread(runnable, "junit-timeout-thread-" +
threadNumber.getAndIncrement());
}
```
No `setDaemon`. `Thread` inherits daemon status from its creator, and the
test thread is non-daemon, so **the abandoned worker is non-daemon** — and a
non-daemon thread parked on an uninterruptible socket read is, on the face of
it, exactly the thing that would keep a JVM from exiting. If that were live,
this PR would trade a 180-minute test hang for a 180-minute *shutdown* hang and
be worse than useless.
It is not live. Failsafe 2.22.2's `ForkedBooter` terminates the fork
explicitly — `exit(int)` and `acknowledgedExit()` both call `System.exit`, and
`kill(int)` calls `Runtime.halt` — so the fork never waits on non-daemon
threads. Verified in the booter bytecode.
I have written it into the Javadoc anyway, because the bound's usefulness
rests on a property of the surrounding harness rather than of this class, and
that is the kind of dependency that silently stops holding after a plugin
upgrade.
## Also documented
`@AfterAll` closes the container the parked thread is still reading from,
per your secondary note — noisy teardown logging rather than a hang, but it
belongs in the same list.
No behaviour change in this commit; the diff is comments and the
description. CI status on the new head as of writing: `Build` is queued.
--
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]
