Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Terminal exits service #19

Closed
MichaIng opened this issue Nov 6, 2021 · 16 comments
Closed

[Bug] Terminal exits service #19

MichaIng opened this issue Nov 6, 2021 · 16 comments
Labels
bug Something isn't working

Comments

@MichaIng
Copy link
Collaborator

MichaIng commented Nov 6, 2021

As we found already on the PR at the DietPi repo. There is an exit signal btw:

Main PID: 50144 (code=killed, signal=HUP)

So a SIGHUP is sent to the main process.

Part of a problem is that in systemd units, SHELL == /bin/sh despite User=root being defined. Not sure how to force it to use users real shell. We should use /bin/bash here hardcoded, since this also it the only shell where the banner and the command shortcuts are known to work. But forcing/setting the variable does not fix the crash, so it is a dedicated issue.

I'm not sure where the SIGHUP signal is coming from, but probably we can force systemd to keep the service active when it is received.


While SIGHUP is often used to reload a service, here it stops it, i.e. kill -HUP <PID> stops the process started form console. So this signal seems to be sent only when it is started via web UI. Not sure if a systemd unit has somehow issues with the PTY 🤔.


I guess (*cmd.write().await).kill().unwrap(); usually kills the shell process only, however, I wonder it this can in some circumstances kill the parent process? The writer does already .write_all("exit".as_bytes()), which should usually be sufficient. Probably we need to add some debug code to check what's going on. I'll now try to run it with strace.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 6, 2021

Lol when wrapped into strace it works fine:

Nov 06 19:36:05 VM-Bullseye strace[52172]: ) = 0
Nov 06 19:36:05 VM-Bullseye strace[52172]: futex(0x5561bf153c80, FUTEX_WAKE_PRIVATE, 1) = 0
Nov 06 19:36:05 VM-Bullseye strace[52172]: accept4(6, {sa_family=AF_INET, sin_port=htons(52193), sin_addr=inet_addr("192.168.1.10")}, [128->16], SOCK_CLOEXEC|SOCK_NONBLOCK) = 12
Nov 06 19:36:05 VM-Bullseye strace[52172]: epoll_ctl(5, EPOLL_CTL_ADD, 12, {EPOLLIN|EPOLLOUT|EPOLLRDHUP|EPOLLET, {u32=6, u64=6}}) = 0
Nov 06 19:36:05 VM-Bullseye strace[52172]: setsockopt(12, SOL_TCP, TCP_NODELAY, [1], 4) = 0
Nov 06 19:36:05 VM-Bullseye strace[52172]: write(4, "\1\0\0\0\0\0\0\0", 8)         = 8
Nov 06 19:36:05 VM-Bullseye strace[52172]: accept4(6, 0x7ffe8a479730, [128], SOCK_CLOEXEC|SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
Nov 06 19:36:05 VM-Bullseye strace[52172]: futex(0x5561bf154c58, FUTEX_WAIT_PRIVATE, 0, NULL
Nov 06 19:36:05 VM-Bullseye strace[52175]: 2021-11-06 19:36:05,104 INFO  [dietpi_dashboard] Request to /ws/term

On every terminal access/reload there is this Resource temporarily unavailable once, not sure whether this has to do something with the issue 🤔?

@ravenclaw900
Copy link
Owner

@MichaIng, if the command is wrapped in nohup, you get the error:
rpi4 nohup[207736]: thread 'tokio-runtime-worker' panicked at 'called `Result::unwrap()` on an `Err` value: Spawn(Os { code: 1, kind: PermissionDenied, message: "Operation not permitted" })',
Is systemd denying something?

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

That matches my thoughts, I had that step in mind already 👍. nohup detaches STDIN/STDERR/STDOUT from the terminal. systemd by default does not attach any terminal in the first place. I tried StandardOutput=tty and StandardInput=tty already, which attaches the service to the local terminal, but this didn't help either for some reason, and it is also not desired to have all service logs on the main console.

systemd does not deny terminal device access generally, but some step around spawning the PTS somehow expects/requires a terminal attached to STDIN and/or STDOUT already. Probably the order needs to be tweaked. I.e. currently the command is spawned without a PTS, respectively without attaching the process/command to that PTS at first, if I understand it right: https://github.com/ravenclaw900/DietPi-Dashboard/blob/abb0c71/src/backend/src/terminal.rs#L21-L26
From the pty_writer the actual PTS/attachment is then done, as part of the pts_resize? However, if /bin/bash is called without STDIN attached to a terminal device/emulator, it exits immediately, which may be the issue here. Probably the exit command/signal is then passed through to parent process while it was meant for the bash process, or so... We need to assure that the PTS is fully established for the shell and that all exit/kill involved is only received by that shell.

I found the docs for the used crate: https://docs.rs/pty-process/0.1.0/pty_process/

If it is somehow difficult to achieve things with it, there seems to be an alternative with uses Tokio explicitly: https://docs.rs/tokio-pty-process/0.4.0/tokio_pty_process/

@ravenclaw900
Copy link
Owner

ravenclaw900 commented Nov 7, 2021

I tried to use tokio-pty-process originally, however it uses features that aren't in Tokio anymore. (It uses version 0.2.9, while the current version is 1.13.0) If needed, we could do it manually using the nix crate. I tried putting debug messages in, and it doesn't even start the other threads, just failing immediately at spawning the PTY.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Ah okay. Probably it works when creating the PTY with a low default size, like:

.spawn_pty(&pty_process::Size::new(80,20))

Btw:

let cmd_read = Arc::clone(&cmd);

does not clone the process or PTY itself but only the reference to the single process and PTS, so that input and output loops can run asynchronously (from each other), right?

@MichaIng MichaIng added the bug Something isn't working label Nov 7, 2021
@ravenclaw900
Copy link
Owner

Nope, still failing, with the same error. Yes, that just clones a reference that can be used in both threads.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Another test would be to explicitly redirect STDIN/STDOUT/STDERR as I'm not sure of other impacts of nohup. Like:

# STDIN only
./dietpi-dashboard < /dev/null
# All streams
./dietpi-dashboard < /dev/null 2>&1 | tee

@ravenclaw900
Copy link
Owner

Just a note: it works with nohup if ran manually, but not in the service. Both of those commands also work.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Right. Not sure how to change that, but regardless of how I wrap commands on a console, they remain "attached" to the shells TTY, when checking via htop (when displaying the TTY column).

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Btw, official systemd documentation: https://www.freedesktop.org/software/systemd/man/systemd.service.html
I already search through it to see whether there is anything about terminal attachment/permissions or such, but couldn't find anything, aside of the mentioned StandardOutput/StandardInput options..

@ravenclaw900
Copy link
Owner

Hmm, setting StandardInput=tty seems to actually make it work for me.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Does it? Strange, it didn't work for me, but probably I messed with something else when testing around. Will retest. I wonder the implications as in theory this means that input on the local console goes to the dashboard. It doesn't react to it, though, what about CTRL+C?

We could however set a TTYPath that is definitely unused, like /dev/tty42 or so 😄. Mid/Long term I would be more happy with a solution that works without any parent TTY, but to have a workaround for DietPi v7.8 this sounds sane.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

It is strange, if I only add StandardInput=tty on my Bullseye VM, the service does not really start up at all: htop shows a somehow broken process name "(ashboard)", the TTY still shows a question mark (if none was attached) and I cannot access the web interface, also not a single log line after "Started Web Dashboard (DietPi)." (also not on /dev/tty1).

Ah adding additionally TTYPath=/dev/tty9 solves this. Probably the default /dev/console and /dev/tty1 is a problem when actively used already as default console, for kernel logs or so? However, also service logs are then done to this console which can then be seen when switching to it via ALT+F9 on the local console, StandardOutput defaults to the value of StandardInput 😄. Solving this again via StandardOutput=journal.

So in combination, this works fine here:

[Unit]
Description=Web Dashboard (DietPi)
Wants=network-online.target
After=network-online.target

[Service]
StandardInput=tty
TTYPath=/dev/tty42
StandardOutput=journal
ExecStart=/opt/dietpi-dashboard/dietpi-dashboard

[Install]
WantedBy=multi-user.target

For you StandardInput=tty only worked? Which hardware/architecture did you try it on? Also, I used the nightly version downloaded by dietpi-software, probably you tested a newer/different build?

@ravenclaw900
Copy link
Owner

ravenclaw900 commented Nov 7, 2021

Indeed, I only needed StandardInput=tty. It was tested on RPi4, aarch64, and I was also using the latest nightly. It did show up on /dev/tty1 for me. However, your configuration also works (and doesn't clutter up the main console). I'll open a PR on the main repo to use it.

Also, since this isn't directly a problem with the dashboard, I'm now going to make a 0.3.1 release with all of the bugs you caught fixed.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Yes makes sense, also since the "Stable" release selection is currently broken due to the architecture suffix change, AFAIK 😄.
EDIT: Ah no, this change was applied with v0.3.0 release already 👍.

MichaIng added a commit to MichaIng/DietPi that referenced this issue Nov 7, 2021
+ DietPi-Software | Fix Terminal on DietPi-Dashboard: ravenclaw900/DietPi-Dashboard#19
@MichaIng
Copy link
Collaborator Author

MichaIng commented Nov 7, 2021

Okay, we may want to find a cleaner solution, but for now it's fine 👍. Marking this as solved/closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants