zach/z-bar-qt
1
1
Fork 0
Code Issues 22 Pull Requests 1 Actions Packages Projects 1 Releases Wiki Activity

hotfix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race condition #96

Merged
zach merged 5 commits from hotfix-restart-race-condition into main 2026-05-24 18:23:43 +02:00
Conversation 4 Commits 5 Files Changed 6
+36 -8
2 changed files with 36 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b49165e7ea - Show all commits
cli/src/zshell/subcommands/__pycache__/shell.cpython-314.pyc
BIN
View File
Binary file not shown.
cli/src/zshell/subcommands/shell.py
+36 -8
View File
@@ -1,5 +1,8 @@
import subprocess
import sys
import time
import click
import typer
args = ["qs", "-c", "zshell"]
@@ -9,40 +12,65 @@ app = typer.Typer()
@app.command()
def kill():
subprocess.run(args + ["kill"], check=False)
result = subprocess.run(args + ["kill"], capture_output=True)
if result.returncode != 0:
raise click.ClickException("No running instance to kill.")
sys.stderr.write(result.stderr.decode())
@app.command()
def start(no_daemon: bool = False):
subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), check=True)
check = subprocess.run(args + ["ipc"] + ["show"], capture_output=True)
zach marked this conversation as resolved Outdated
zach commented 2026-05-24 01:45:54 +02:00
Outdated
Review
Copy Link
Copy Source

The start command should not need to run qs -c zshell ipc call, IPC calls are used to trigger states and functions within the QML code. The -n flag already checks if there's a running instance of the shell.

The start command should not need to run `qs -c zshell ipc call`, IPC calls are used to trigger states and functions within the QML code. The `-n` flag already checks if there's a running instance of the shell.
AramJonghu commented 2026-05-24 02:41:36 +02:00
Outdated
Review
Copy Link
Copy Source

Was not aware of this. Remaining functions use ipc, which is why I did not bat an eye. Resolving now.

Was not aware of this. Remaining functions use ipc, which is why I did not bat an eye. Resolving now.
AramJonghu commented 2026-05-24 02:46:27 +02:00
Outdated
Review
Copy Link
Copy Source

Change occurred since I wanted an error block such as in the screenshot. But with that info, ipc is indeed not needed in the code.

Change occurred since I wanted an error block such as in the screenshot. But with that info, ipc is indeed not needed in the code.
if check.returncode == 0:
raise click.ClickException("An instance of this configuration is already running.")
result = subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), capture_output=True)
if result.returncode != 0:
raise click.ClickException(result.stderr.decode().strip())
sys.stderr.write(result.stderr.decode())
@app.command()
def restart(no_daemon: bool = False):
subprocess.run(args + ["kill"], check=False)
subprocess.run(args + ["kill"])
for _ in range(50):
AramJonghu marked this conversation as resolved Outdated
zach commented 2026-05-24 01:49:33 +02:00
Outdated
Review
Copy Link
Copy Source

This seems ugly. Personally think we should just set a sleep timer of ~5 seconds, then run code.

This seems ugly. Personally think we should just set a sleep timer of ~5 seconds, then run code.
AramJonghu commented 2026-05-24 02:56:15 +02:00
Outdated
Review
Copy Link
Copy Source

Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense.

time.monotonic() could be used to hide the ugliness, but same logic/idea.

Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense. time.monotonic() could be used to hide the ugliness, but same logic/idea.
zach commented 2026-05-24 10:42:10 +02:00
Outdated
Review
Copy Link
Copy Source

Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available.

Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available.
AramJonghu commented 2026-05-24 13:56:45 +02:00
Outdated
Review
Copy Link
Copy Source

Is that not okay for a rare command as restart?

We could change the loop to check every 250ms instead, so it is 10 checks instead of 50.

Is that not okay for a rare command as restart? We could change the loop to check every 250ms instead, so it is 10 checks instead of 50.
zach commented 2026-05-24 17:53:02 +02:00
Outdated
Review
Copy Link
Copy Source

That's a good middle ground I think, agreed.

That's a good middle ground I think, agreed.
AramJonghu commented 2026-05-24 18:05:21 +02:00
Outdated
Review
Copy Link
Copy Source

Changed it.

Changed it.
result = subprocess.run(args + ["kill"], capture_output=True)
if result.returncode == 255:
break
time.sleep(0.05)
subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), check=True)
result = subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), capture_output=True)
zach marked this conversation as resolved Outdated
zach commented 2026-05-24 01:50:31 +02:00
Outdated
Review
Copy Link
Copy Source

Here we can just call start(), right? Not sure if Typer allows calling command functions.

Here we can just call `start()`, right? Not sure if `Typer` allows calling command functions.
AramJonghu commented 2026-05-24 03:08:07 +02:00
Outdated
Review
Copy Link
Copy Source

It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a start_instance() that start uses outright, and is reused by restart. This could satisfy testing and reuse of code, and perhaps future code changes.

It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a `start_instance()` that start uses outright, and is reused by restart. This could satisfy testing and reuse of code, and perhaps future code changes.
if result.returncode != 0:
raise click.ClickException(result.stderr.decode().strip())
sys.stderr.write(result.stderr.decode())
@app.command()
def show():
subprocess.run(args + ["ipc"] + ["show"], check=True)
result = subprocess.run(args + ["ipc"] + ["show"], capture_output=True)
if result.returncode != 0:
raise click.ClickException(result.stderr.decode().strip())
sys.stderr.write(result.stderr.decode())
@app.command()
def log():
subprocess.run(args + ["log"], check=True)
result = subprocess.run(args + ["log"], capture_output=True)
if result.returncode != 0:
raise click.ClickException(result.stderr.decode().strip())
sys.stdout.write(result.stdout.decode())
sys.stderr.write(result.stderr.decode())
@app.command()
def lock():
subprocess.run(args + ["ipc"] + ["call"] + ["lock"] + ["lock"], check=True)
result = subprocess.run(args + ["ipc"] + ["call"] + ["lock"] + ["lock"], capture_output=True)
if result.returncode != 0:
raise click.ClickException(result.stderr.decode().strip())
sys.stderr.write(result.stderr.decode())
@app.command()
def call(target: str, method: str, method_args: list[str] = typer.Argument(None)):
subprocess.run(args + ["ipc"] + ["call"] + [target] + [method] + (method_args or []), check=True)
result = subprocess.run(args + ["ipc"] + ["call"] + [target] + [method] + (method_args or []), capture_output=True)
if result.returncode != 0:
raise click.ClickException(result.stderr.decode().strip())
sys.stderr.write(result.stderr.decode())