larkery / zsh-histdb

A slightly better history for zsh
MIT License
1.27k stars 75 forks source link

sqlite3 prevents Terminal from closing on macOS #83

Closed sumanthratna closed 4 years ago

sumanthratna commented 4 years ago

Reproduction

  1. open Terminal.app in macOS
  2. close the terminal window you just opened
  3. there's this popup: Screen Shot 2020-07-01 at 10 00 15

Of course, I prefer running exit instead of clicking the close button through the GUI. However, this is an inconvenience because when I shutdown my computer while leaving a terminal window open, I get the popup which disturbs the shutdown. Further, my quit alias also doesn't work because of the popup.

Is there a way to label sqlite3 as a background process or something like that so the terminal can end the sqlite3 process on its own?

larkery commented 4 years ago

I have no idea about this I'm afraid, I do not use MacOS. What should happen is that clicking close sends SIGTERM to the shell, which involves zshexit which terminates the sqlite process. It might be that Terminal has a whitelist of processes not to complain about, as there is no fundamental difference between the sqlite process and the shell process that I can think of.

larkery commented 4 years ago

this link suggests there's a whitelist you can fiddle with.

sumanthratna commented 4 years ago

I use powerlevel10k and somehow it exits on its own when I close the window. https://github.com/romkatv/powerlevel10k/blob/master/internal/worker.zsh might have to do with it. https://github.com/mafredri/zsh-async might also help, but I'm not sure if zsh-histdb can include it as a dependency.

EDIT: https://github.com/larkery/zsh-histdb/blob/eeef11248b44e6a203d2e2cf796f2b5b548da489/sqlite-history.zsh#L31-L47 Strange, it looks like you've already tried to fix this.

sumanthratna commented 4 years ago

Maybe trap will help?

davea commented 4 years ago

I've just opened PR #84 which will hopefully fix this.

larkery commented 4 years ago

Sounds like this is fixed.

zsh-async looks clever, but I don't want to add a dependency.

I think the example code from powerlevel10k essentially does same thing as what the code in there does, except I am not sure they properly trap the condition where the child process dies and the pid is recycled. However there's a lot of mystic invocations in there so maybe one of them deals with it, I don't know.

I would happily accept a better way of reliably terminating the sqlite process on exit if anyone has one. I had thought that closing the pipe for writing would cause sqlite3 to see EOF and stop, but it doesn't. I'm not sure why that is.

Anyway, thanks chaps, bug fixed for now.

larkery commented 4 years ago

Actually I did a bit of reading and it seems like the answer is:

I think this is fixable by invoking sqlite3 against the fifo directly, so it doesn't inherit the write-end of the pipe, but I need to do some tests.

sumanthratna commented 4 years ago

Ugh—I just updated and this issue is still occurring. I think I said that #84 worked earlier because I didn't hit enter, so sqlite3 never started in the first place (aka I'm a fool :) )

FWIW, typing in exit works without complaints.

larkery commented 4 years ago

I wonder whether the reason you don't get the issue with powerlevel10k is because that spawns zsh as its subprocess, which is already on the whitelist of things terminal doesn't care about?

Since the dialog comes up when you click close I think adding to the whitelist must be the only option. I can't think of anyway terminal can introspect the processes spawned by the terminal and detect that they will terminate later!

larkery commented 4 years ago

Just to make it a bit clearer: nothing in the cleanup hook can be responsible, because the dialog is shown before the cleanup hook is triggered. This is obvious from the fact that you can press cancel and it doesn't happen! So we would need the terminal to somehow detect that sqlite3 is not something it cares about, which is I think solved by the whitelist preference I linked above.

sumanthratna commented 4 years ago

Thanks for the detailed explanations.

I wonder whether the reason you don't get the issue with powerlevel10k is because that spawns zsh as its subprocess, which is already on the whitelist of things terminal doesn't care about?

I'm not sure about this. The whitelist label says "Only if there are processes other than the login shell..." and I'm pretty confident macOS wouldn't add zsh to that list without mentioning it. I think powerlevel10k is the login shell, it doesn't spawn a new process. I can confirm this by spawning a new zsh shell within the login shell—clicking the close button causes the prompt to show up for both sqlite3 and zsh as opposed to just sqlite3.

I've added sqlite3 to the whitelist, but I'm wondering if this is a bad idea. Considering that screen and tmux are the defaults, this whitelist probably means that sqlite3 won't ever be killed when I close my terminal windows. This could be problematic, because over time, my CPU could get cluttered with loose sqlite3 processes, unless macOS garbage collects such processes.

If nothing works out, it might be useful to programmatically add sqlite3 to the whitelist using the defaults command (of course, we should ask for permission from the user first).

larkery commented 4 years ago

The sqlite3 processes due to zsh-histdb should get killed when the shell is terminated. powerlevel10k does start a new zsh process around here https://github.com/romkatv/powerlevel10k/blob/e0ed693e6dcf01a1e0d104ca10d23d1a76b89153/internal/worker.zsh#L186 but it's a subshell which might make a difference. I guess we'd have no know more about how terminal works.

larkery commented 4 years ago

Reading some manual pages to try and understand a bit more about how you might detect the login shell, I see that one convention is that argv[0] starts with a dash (so like -zsh or -bash or so). It might be that starting sqlite3 with a dash at the front of its name would make terminal quiet.

I think you can try this directly by running something like

exec -l sqlite3 blah.sqlite

and then try closing the window and see if it complains. As an extreme measure could run exec -a -zsh sqlite3 somefile.sqlite which would set argv[0] to -zsh, but that seems horrible.

sumanthratna commented 4 years ago

What do you expect the output to look like? When I run the commands you specified, I get an interactive SQL prompt, so Terminal does complain when I try closing it (which I would fully expected). Note: I didn't run your commands on valid SQL files (neither blah.sqlite nor somefile.sqlite existed).

larkery commented 4 years ago

Terminal has this setting to ask you before closing the window if there is something running other than "the login shell", in the preferences page I linked.

What I am wondering is how it knows what "the login shell" is, so we can make it not complain.

Options I was imagining were:

If none of these work and terminal does have some way to see that a subprocess is "part of" the login shell it must be something else. I can't think of any good way it could be doing this, and we can't see the code so it is tough to make any progress.

sumanthratna commented 4 years ago

In my terminal (the comments below commands are the outputs):

ps -A | grep sqlite3
# 11106 ttys004    0:00.01 sqlite3 -batch /Users/suman/.histdb/zsh-history.db
# 11207 ttys004    0:00.00 grep sqlite3

ps -o args= -p $HISTDB_SQLITE_PID | read -r args
echo $HISTDB_SQLITE_PID
# 11106

kill -TERM $HISTDB_SQLITE_PID
# kill: kill 11106 failed: no such process

ps -A | grep sqlite3
# 11740 ttys004    0:00.00 grep sqlite3

from: https://github.com/larkery/zsh-histdb/blob/6750a1713bf5a56c234ae24bcdb2ce65e66b76d2/sqlite-history.zsh#L31-L45

It looks like _histdb_stop_sqlite_pipe should be killing sqlite3 but for some reason it complains anyway.

larkery commented 4 years ago

_histdb_stop_sqlite_pipe won't be called until after you press Terminate, so it's not relevant here.

The problem is that on closing the window, before Terminal does anything else, it looks at the processes that are running and decides whether to show you the dialog box.

If you say "Cancel", it then does nothing and zsh + children never even know you had considered closing the window. If you click "Terminate", Terminal will send some signals to the processes it considers attached to that window (probably SIGTERM followed by SIGKILL if they don't go away), in which case the sqlite process ought to get shutdown by _histdb_stop_sqlite_pipe. However that can only happen after the dialog has displayed and you have chosen "Terminate"

So, what I am wondering is what heuristic Terminal uses to decide when the processes it can see should cause it to show the warning dialog, and if we can convince it that our sqlite3 is actually part of "the login shell", which we know it doesn't complain about.

Whether _histdb_stop_sqlite_pipe works is a separate issue to whether this dialog can be prevented.

sumanthratna commented 4 years ago

Right, that makes sense. Running exec -a -zsh sqlite3 somefile.sqlite & and then closing results in a prompt for -zsh but not sqlite3. Running exec -l sqlite3 blah.sqlite & results in a prompt for sqlite3 and -sqlite3.

Also, I think this issue should be reopened since it's still occurring (I don't expect you to write a PR yourself, though).

Just leaving these here:

cat /etc/shells:

# List of acceptable shells for chpass(1).
# Ftpd will not allow users to connect who are not using
# one of these shells.

/bin/bash
/bin/csh
/bin/dash
/bin/ksh
/bin/sh
/bin/tcsh
/bin/zsh

ps aux | grep sqlite3:

suman            24566   0.0  0.0  4268316    564 s004  R+   10:01AM   0:00.00 grep sqlite3
suman            14138   0.0  0.0  4297664   1028   ??  SN    9:48AM   0:00.01 sqlite3 -batch /Users/suman/.histdb/zsh-history.db
suman            12415   0.0  0.0  4297664   1040   ??  SN    9:46AM   0:00.01 sqlite3 -batch /Users/suman/.histdb/zsh-history.db

Is there a reason there are two sqlite3 processes? I would assume only 1 is necessary.

larkery commented 4 years ago

If you have two shells open you would have two, or if one has not been killed cleanly for some reason. I have a change which will improve that which I should push later on.

My suspicion here (from reading the other link also) is that Terminal has a simple behavior that we can't work around: if the "warn on closing" preference is set, then it will warn you about any live process started under that terminal where it is not either:

  1. The login shell process, checked by seeing if it has the same pid
  2. On the whitelist of things to not complain about

I would just stick it on the whitelist and be done with it. After all, this should just kill sqlite3 when you shut the window without showing you a message, which is probably what you want. The only change this would produce for you is if you wanted to use sqlite interactively, and you wanted terminal to warn you because you sometimes close windows by mistake.

You could change the sqlite invocation to give it a special name which would remove that possibility as well, I suppose.

sumanthratna commented 4 years ago

When I comment this line: https://github.com/larkery/zsh-histdb/blob/4274de7c1bca84f440fb0125e6931c1f75ad5e29/sqlite-history.zsh#L123

This issue is resolved. Running exit seems to increment the count of the command according to histdb-top, so I think zsh-histdb works. @larkery do you think this will cause any unintended consequences?

local cmd="'$(sql_escape $cmd)'"
local pwd="'$(sql_escape ${PWD})'"
local started=$(date +%s)
- _histdb_init
+ # _histdb_init

if [[ "$cmd" != "''" ]]; then
    _histdb_query_batch <<EOF &|