ctrl + c not functional

[GottZ]

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behavior:

1. ping google.de
2. ctrl + c

Expected behavior
termination of ping execution

Screenshots & Logfiles

(killed through a different shell using htop)

Versions:

• Adapter version: 0.2.2
• JS-Controller version: 4.0.21
• Node version: v14.19.1
• Operating system: docker: buanet/iobroker:latest-v6

Additional context
It is impossible to type in new commands while the ping is running.

[GottZ]

ok so this is where the signals are handled in the backend.

ioBroker.terminal/lib/web-terminal/lib/terminal.js

Lines 276 to 296 in 18b25fb

	socket.on("signal", function (signal) {
	var cmd;
	if (replSrv) {
	switch (signal) {
	case "SIGINT":
	cmd = ".break";
	break;
	case "SIGQUIT":
	cmd = ".exit";
	break;
	}
	stdin.write(cmd + linebreak);
	} else if (proc) {
	if (process.platform === "win32") {
	spawn("taskkill", ["/pid", proc.pid, '/f', '/t']);
	} else {
	proc.kill(signal);
	}
	}
	});

and this is where the signals are emitted from the frontend to the backend.

ioBroker.terminal/lib/web-terminal/web/init.js

Lines 373 to 389 in 18b25fb

	if (e.ctrlKey) {
	var charCode = (typeof e.which === "number") ? e.which : e.keyCode;
	switch (charCode) {
	case 67:
	e.stopImmediatePropagation();
	socket.emit("signal", "SIGINT");
	appendContent("^C");
	break;
	case 68:
	e.stopImmediatePropagation();
	socket.emit("signal", "SIGQUIT");
	appendContent("^D");
	break;
	}
	}

67 is indeed ctrl + c and 68 is ctrl + d.
notice how ever that ctrl + d is used to save your current website as bookmark to the browser.

[GottZ]

SIGINT is submitted to the backend on pressing ctrl + c but seems to be unable to interrupt the currently running process.
SIGQUIT how ever is not emitted at all. (current microsoft edge.. basically chrome)

Interrupting a running process by executing kill -2 on the pid (sending sigint) works fine.

/opt/iobroker$ ps aux | grep ping

iobroker   45205  0.0  0.0   2420   520 ?        S    10:06   0:00 /bin/sh -c ping google.de
iobroker   45206  0.0  0.0   7656  2160 ?        S    10:06   0:00 ping google.de
iobroker   45352  0.0  0.0   2420   588 ?        S    10:09   0:00 /bin/sh -c ps aux | grep ping
iobroker   45354  0.0  0.0   6496   724 ?        S    10:09   0:00 grep ping
/opt/iobroker$ kill -2 45206

/opt/iobroker$ ps aux | grep ping

iobroker   45369  0.0  0.0   2420   520 ?        S    10:09   0:00 /bin/sh -c ps aux | grep ping
iobroker   45371  0.0  0.0   6496   716 ?        S    10:09   0:00 grep ping
/opt/iobroker$  

So I assume the problem lies here:

ioBroker.terminal/lib/web-terminal/lib/terminal.js

Line 293 in 18b25fb

proc.kill(signal);

[GottZ]

ok so apparently replacing

ioBroker.terminal/lib/web-terminal/lib/terminal.js

Line 293 in 18b25fb

proc.kill(signal);

with

socket.emit("console", `killing ${proc.pid} with signal ${signal} ${proc.kill(signal) ? "succeeded" : "failed"}`);

does indicate that the kill succeeds.

/opt/iobroker$ sleep 5

^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT succeeded
^C
killing 2536 with signal SIGINT failed
^C
killing 2536 with signal SIGINT failed
^C
killing 2536 with signal SIGINT failed
^C
killing 2536 with signal SIGINT failed

I assume sigint just does not propagate through sh -c
Let's see if that is true:

ping google.de in one console.
ps aux returns this:

iobroker    2809  0.0  0.0   2420   588 ?        S    11:22   0:00 /bin/sh -c ping google.de
iobroker    2810  0.0  0.0   7656  2464 ?        S    11:22   0:00 ping google.de

the SIGINT will be sent to pid 2809.

^C
killing 2809 with signal SIGINT succeeded

Executing kill -2 2809 yields the same effect, and does not stop the ping.

The problem boils down to using /bin/sh, which will not forward signals to child processes.
In worst case, /bin/sh will continue to stay alife in case the surrounding nodejs process was killed by SIGKILL. (A Problem I encountered years ago)

Related issues with workarounds:

nodejs/node#40438

sindresorhus/execa#96

From my experience, the problem boils down to using sh as wrapper for executing commands that could technically be executed without it.

This Terminal Plugin should probably just abandon this legacy terminal thing and move over to something like https://github.com/microsoft/node-pty with https://github.com/xtermjs/xterm.js as frontend (vscode uses it)

I technically could fix the problems by just removing /bin/sh from this execution chain here but why worry, if a better dependency exists.
After all allocating a ptty would be a more elegant solution for a terminal application.
It would also fix upstream issues like:
rabchev/web-terminal#1
rabchev/web-terminal#2
rabchev/web-terminal#7
and various others, not even opened yet.

want me to try refactoring this?

https://github.com/tsl0922/ttyd
appears to fulfill my needs just perfectly.
porting it to iobroker wouldn't be a big problem.

[Apollon77]

Hi,

thank you very much for all these investigations!

We already have an xterm adapter you might want to try out which should base exactly on that ... SO for terminal I would also be happy for a PR for a fix, but would not refacter too much. because maybe xterm is a better way for the future

[GottZ]

xterm doesn't seem to be in the stable repo. is there anything blocking it from getting there?
I consider ioBroker.terminal completely deprecated now that I know this.

I'll play a bit with it to fix ctrl + c tho, but not today.

[Apollon77]

xterm is in beta repo ... on a list to bring to stable tho. You can install from beta (Admin - enable expert mode - custom install button - install from npm)

[Apollon77]

@GottZ So after reading all the input I think the "Only" solution that makes sense to me would be to use https://github.com/simonepri/pidtree with proc.pid and then kill all that came back there. WHat do you think?

[GottZ]

Oh that's dirty! I like it.
Yep that should indeed work.

[Apollon77]

Do you want to test it :-) GitHub is updated ... please restart adapter once after update

[GottZ]

nice. it works flawless on linux.
I guess this solves this Issue then.
Moving to xterm now.
Feel free to close this as soon as it is within the release.