fix(process): do not start a shell for a session that is already gone - #372
fix(process): do not start a shell for a session that is already gone#372kshivang wants to merge 1 commit into
Conversation
Seen in a live install. Updating terminal-tab reloaded the plugin, and during
the teardown a pty spawn arrived at the closed classloader:
ClassNotFoundException: Plugin classloader for '...terminaltab' is
UNLOADED; refusing to resolve 'com.pty4j.util.PtyUtil' against the host
classloader. Something still referenced the plugin after it was unloaded.
at com.pty4j.unix.PtyHelpers.<clinit>
at DesktopProcessService.spawnProcess
at TabController$initializeTerminalSession$1.invokeSuspend
The first pty spawn in that classloader happening during its own teardown, so
PtyHelpers ran its static initialiser against a loader that was already closed.
Three faults, each fixed and each pinned by a test that fails without it.
The spawn was attempted for a cancelled session. `TerminalTab.dispose` does
cancel `coroutineScope`, but cancellation is cooperative and
`PtyProcessBuilder.start()` is a blocking JNI chain with no suspension point,
so a coroutine already inside it never notices. Session init does real work
first - environment assembly, shell-integration injection - and that whole span
is when a tab can be disposed. `ensureActive()` before the call narrows it to
an instruction gap. It does not close it, and nothing here claims otherwise.
`catch (e: Exception)` did not catch the failure. A closed classloader raises
NoClassDefFoundError / ExceptionInInitializerError, which are Errors, so the
documented "returns null on failure" contract broke in precisely the case that
produced the stack above: instead of a connection error on the tab, the
throwable escaped to the host. Now `catch (t: Throwable)`.
Cancellation would have been swallowed by that same clause and reported as
"Failed to spawn process" on a tab already on its way out, so the
CancellationException clause sits above it and rethrows.
The pty start is injectable now, for one reason: a LinkageError cannot be
provoked through the real PtyProcessBuilder, so without a seam the
Throwable-vs-Exception distinction is untestable - and an untested catch clause
is how it came to be Exception-only. Verified by reverting each of the three
changes separately; each fails exactly one test and no others.
Deliberately not done: eagerly warming the pty4j classes at startup would
remove this stack entirely, but it costs class loading and a JNA library load on
every launch for users who never open a terminal. The two guards above make a
late spawn resolve cleanly instead.
ReviewThe diagnosis is right and the write-up is unusually good — the three faults are genuinely distinct, the clause ordering ( Two things I'd want fixed before merge, plus some smaller notes. 1.
|
1 participant
Seen in a live install
Updating terminal-tab reloaded the plugin, and during the teardown a pty spawn arrived at the already-closed classloader:
The host's guard was right to refuse. The first pty spawn in that classloader happened during its own teardown, so
PtyHelpersran its static initialiser against a loader that was already closed.Three faults, not one
1. The spawn was attempted for a cancelled session.
TerminalTab.disposedoes callcoroutineScope.cancel(). But cancellation is cooperative andPtyProcessBuilder.start()is a blocking JNI chain with no suspension point, so a coroutine already inside it never notices. Session init does real work before that call - environment assembly, shell-integration injection - and that whole span is when a tab can be disposed.currentCoroutineContext().ensureActive()before the call narrows the window from all of session init to an instruction gap. It does not close it, and nothing in the code or the tests claims otherwise.2.
catch (e: Exception)did not catch the failure.A closed classloader raises
NoClassDefFoundError/ExceptionInInitializerError. Those are Errors, not Exceptions, so the documented "returns null on failure" contract broke in exactly the case that produced the stack above: rather than a connection error on the tab, the throwable escaped to the host. Nowcatch (t: Throwable).3. Cancellation would have been swallowed by that same clause and reported as "Failed to spawn process" on a tab already on its way out. The
CancellationExceptionclause sits above it and rethrows.Why the pty start is now injectable
One reason: a
LinkageErrorcannot be provoked through the realPtyProcessBuilder, so without a seam the Throwable-vs-Exception distinction is untestable - and an untested catch clause is how it came to be Exception-only in the first place. The default argument is the original builder chain, unchanged.This was not hypothetical caution. My first version of the cancellation test used
withContext(cancelledJob), which throws at thewithContextboundary beforespawnProcessruns, so it passed against the Exception-only catch and proved nothing. The seam is what made both properties real.Tests
1089 compose-ui tests, all green. Verified by reverting each change separately - each fails exactly one test and no others:
catch (t: Throwable)→catch (t: Exception)a linkage error resolves to null rather than escapingCancellationExceptionclausecancellation from inside the spawn propagatesensureActive()a cancelled caller does not get a processI also deleted a test I had written with
assertFalse(false, "placeholder…")in it. It asserted nothing; the limitation it was gesturing at is stated in the KDoc instead.Deliberately not done
Eagerly warming the pty4j classes at startup would remove this stack entirely, since
<clinit>could never run late. It costs class loading plus a JNA library load on every launch for users who never open a terminal, which is the wrong trade for a startup path. The two guards make a late spawn resolve cleanly instead of crashing.