Importing OpenTUI stops Effect from properly running teardown hooks

[limwa]

Description

As the title says, simply importing @opentui/core disables all teardown hooks on Effect (or any other library, for that matter).

Example

import { Effect } from "effect";
import { BunRuntime } from "@effect/platform-bun";

// Import opentui with side-effects
import "@opentui/core";

const program = Effect.async<void, Error>((resume) => {
    console.log("Running effect...");

    // Complete the effect after 10s
    const id = setTimeout(() => {
        console.log("Effect completed normally!");
        resume(Effect.void);
    }, 10000);

    // Return an effect to run if the fiber is interrupted
    return Effect.sync(() => {
        console.log("Effect was interrupted!");
        clearTimeout(id);
    });
});

BunRuntime.runMain(program);

Expected behavior

If I press CTRL+C while this program is running, I would expect the logs to be as follows:

Running effect...
^CEffect was interrupted!

Observed behavior

If I press CTRL+C while this program is running, the log is as follows:

Running effect...
^C

Proposed fixes

I've already determined that the cause is the following code snippet:

opentui/packages/core/src/renderer.ts

Lines 132 to 138 in 53017b6

	singleton("ProcessExitSignals", () => {
	;["SIGINT", "SIGTERM", "SIGQUIT", "SIGABRT"].forEach((signal) => {
	process.on(signal, () => {
	process.exit()
	})
	})
	})

I'm only speaking for myself here, but I was not expecting that importing @opentui/core would have these kinds of side effects, and I initially thought it was a problem with my effect code.

Here are some proposals for dealing with this issue:

1. Use process.exitCode = 0; instead of process.exit().

• This does not immediately cause bun to exit, and effect runs all teardown hooks, as expected. This is a simple fix that works for effect, and users can still use process.on("beforeExit", ...) and process.on("exit", ...).
• However, as far as I am aware, after setting exitCode, it is impossible to stop the runtime from terminating. As such, this might still be a problem for some users who might want to ignore SIGINT and some other signals.
• Furthermore, this fix still doesn't eliminate side effects when importing @opentui/core.

2. OR: do not add the event handlers for the process exit signals.

• If this is needed for some sort of cleanup, or to allow the process to be shut down when these signals are received, I would propose that a new option can be added to the render function, which would allow users to specify what to do when these signals are received.
• This option could have as the default implementation (signalType: ...) => process.exitCode = 0;.

3. OR: put this singleton under an environment variable.

(I'd like to note that I'm not really familiar with the opentui codebase, and I don't know how much effort these ideas would take to implement. These are just what I thought of when writing this issue!)

Thanks!

[kommander]

So my perspective was things run on/in opentui, not opentui on/in another framework/lib. That's obviously one sided.

What I could see working here is defining a shutdown behaviour that can be overridden. Question then just is, what should be default and how do users know that?

Could be that createRenderer expects a mandatory Behaviour and it does not default to any. Would force to think about it when using it in a project that handles such things. Is a hurdle for onboarding though, however as for the "build ON opentui" perspective people just copy paste the example that is set up with the current behavior or use bunx create tui. That could cause the same problem again, because people copy that into a project.

Maybe there is a path for not registering the signal handlers at all for opentui and let application level handle that. Mhh yeah I can see that working. Opentui just needs to detach event listeners and shutdown workers to allow the process to exit, but that can be done when the exit is triggered from app level. I think I would go down that path.

Open to suggestions

[limwa]

I looked a bit more into this, and simply removing the signal handlers works (ish), but it has some likely undesired effects:

• if useAlternativeScreen is true, and a signal is received, the terminal is not switched back to the normal screen buffer. That's because the exit event is not fired, and the renderer is not destroyed.
• for the same reason, if useMouse is true, the terminal will continue to report the mouse position, even after the process exits.

To complement my thoughts below, I'll say that, in my opinion, it is expected that opentui should have exclusive control over stdin and stdout, but not over the process lifecycle.

---

What I could see working here is defining a shutdown behaviour that can be overridden. Question then just is, what should be default and how do users know that?

I think the most critical behavior to define is whether process.exit(...) is called. This is what should be overridable.

IMO, a good default would be to destroy the renderer and not to exit the process. In most situations, destroying the renderer would end up exiting the process either way.

Could be that createRenderer expects a mandatory Behaviour and it does not default to any. Would force to think about it when using it in a project that handles such things.

Yeah, a Behavior passed to createCliRenderer would be interesting, and could allow users to decide what signals to handle and/or what to do to shut down the application. Like I said above, I think it can default to destroying the renderer. Since there are projects that have already been written with "I'm building ON opentui and I want it to control the lifecycle as well" in mind, I think that using "exit the process" as a default behavior is also okay.

Again, because I don't think opentui should control the lifecycle, I think that the signal handlers should only be registered while the renderer is active. Furthermore, if the renderer is not active, the signal handlers are not even needed at all, because the terminal is not in any weird states (reporting mouse position, alternative screen buffer, etc.).

Opentui just needs to detach event listeners and shutdown workers to allow the process to exit, but that can be done when the exit is triggered from app level. I think I would go down that path.

Recalling what I said above, the signal handlers should be used to prevent the terminal from remaining in weird states, but IMO, they should only be registered when the terminal is active. The signal handlers ideally would focus on destroying the renderer, but exiting the process is also an option.

[limwa]

Another very useful improvement would be to return the renderer in render from @opentui/react (and other packages alike). This would allow users to implement external shutdown logic and more easily get it to destroy the renderer (currently, I need to create a React component that calls useRenderer and, on mount, adds an event listener to an AbortSignal that destroys the renderer).

[kommander]

Yes to returning the renderer and yes to "opentui should not control the lifecycle", that is app responsibility.

I think the Behaviours would get way too complex and complicated. If the renderer does just not try to control the lifecycle I think it can be very much simplified.

So while the responsibility to destroy the renderer would be on app level, the renderer could take a list of signals it reacts to, to shutdown properly. Default what it listens to now as well. The signal list can then be set to null or empty in the options to tell the renderer to just do nothing on signals and the app has full control then on when to destroy the renderer.

There are some other process.exit listeners that are probably fine though because I think it's just to restore console and whatnot, which is irrelevant if the process is going to die anyway.

[limwa]

So, would the API be something like the following?

const { renderer } = render(
	<App />,
	{
		shutdownSignals: ["SIGINT", "SIGABRT"],
    }
);

If so, this is good. I think I can implement this and create a PR in 1 or 2 days.

[kommander]

Yes, that would be it basically.