lf -server doesn't quit automatically

doronbehar commented 5 years ago

This is somewhat related to #106, but I'm using GNU/Linux.

I noticed that when I login from a tty and run lf inside it, even if I don't do any file operations and I just browse some directories, the lf -server process stays running in the background forever. The unfortunate impact of this behavior, is that the process stays running even when I logout which makes all the user services systemd runs for me keep running (pulseaudio, ssh-agent etc.). I also have an encrypted home folder which stays un-encrypted after I logout because of this single process.

Is this behavior intentional? I know I can killall lf on logout or alias lf to something likelf; lf -remote quit` but that's kind of ugly and potentially unsafe if there are some pending file operations right? Maybe the server should be aware of how many clients it has connected and if it has none and there are no pending operations, it should quit by himself.

Currently, if I want to logout and login again from some reason, after I logout I have to login as root, killall lf and login again as the normal user. This is rather tedious :)

Thanks again for creating Lf, I very much enjoy it besides that :)

gokcehan commented 5 years ago

@doronbehar This is somewhat intentional. It has been proposed before to quit after the last client but then the saved file names are lost when you relaunch. You can use a custom quit command to kill the server as such to make things a little easier for you:

map Q &lf -remote 'send quit'; lf -remote 'quit'

I'm curious though, why does systemd not kill the lf process on logout. I feel like this should be the case.

doronbehar commented 5 years ago

I've had encountered similar issues with processes which are left outside a specific process tree and remain stale after logout, see. I don't think systemd could differentiate between a process you'd actually want to get killed and a process you wouldn't like to be killed - think about tmux sessions.

It has been proposed before to quit after the last client but then the saved file names are lost when you relaunch.

What do you mean by 'saved file names'? Perhaps these could be saved in a file instead?

ALX99 commented 4 years ago

Also looking for insight as to why the server can't be closed when the last client exits. What saved file names are lost?

doronbehar commented 4 years ago

What saved file names are lost?

I think the intention was file names you marked for copy / move. Looking at the labels of the issue, I think @gokcehan has concluded without words that it would be better to make the server quit when the last client dies, but also save the list of files marked for copy / move in a file.

gokcehan commented 4 years ago

@doronbehar Sorry, it seems that I forgot to drop a reply the last time I was here. I think I was thinking of checking systemd behavior to see why this happens. Anyway, I guess quitting after the last client fits our philosophy better so I don't mind a change.

@ALX99 Sorry for late reply. I see you already have a PR. Thanks for working on this. To be honest, lately I was thinking of getting rid of server involvement to record the names of copy/cut files to keep them in a file at all times (maybe something like ~/.local/share/lf/files). I have seen some users trying to implement custom commands to manipulate these lists (e.g. #377 #388), and I think it would be easier to manipulate a file in these custom commands. Also, it could be possible to add a keybinding to review the file list in an editor. What do you think about this?

Also, I have checked your PR and I'm not sure about the cleanup. Checking for file existence is an anti-pattern in Go. Instead, you're supposed to use the file and check for errors (i.e. "ask forgiveness not permission"). This is the reason there is no fileExists function in standard library. I tried to avoid adding such a function for the same reason, though we have this anti-pattern leaked to the codebase in a few places. One thing to be careful here is that !os.IsNotExist(err) does not necessarily mean that the file exists. So your cleanup commit changes the behavior of the code in some places. I feel like this was unintentional on your side. If you want to work on such a cleanup, can you do that in a separate PR (before or after this PR) so it would be easier to review?

ALX99 commented 4 years ago

@gokcehan Removing the server altogether is fine by me, I personally did not know of its existence before finding it in my process list, nor knew that selected files were synced between clients. Do you intend to want to implement the same "syncing behavior" with the serverless architecture? For me personally, I've found no use for this feature in my workflow and can't imagine other people have either (correct me if I'm wrong). Rather, it was an annoyance at first when I couldn't understand why the unselect command did not unselect files marked for deletion/moving. Being able to edit the selected files in an editor doesn't sound too bad. In any case, I think it would be a good idea to start a new branch where one could experiment with removing the server.

Regarding the cleanup, I had no idea about it so thanks for informing me! I will create another PR if I find other things that might make the code simpler.

doronbehar commented 4 years ago

:-1: on removing the server feature. I find it useful for:

# Edit the current filename
map ra ${{
    # get 'basename' of the selection
    filename="${f##*/}"
    # quote it so we won't deal with quotes in the lf -remote command
    filename="$(printf '%q' "$filename")"
    filename="${filename// /<space>}"
    lf -remote "send $id push :rename<space>$filename"
}}
# Edit filename before the extension
map re ${{
    # get 'basename' of the selection
    filename="${f##*/}"
    # quote it so we won't deal with quotes in the lf -remote command
    filename="$(printf '%q' "$filename")"
    filename="${filename// /<space>}"
    lf -remote "send $id push :rename<space>$filename<a-b><c-b>"
}}

And for this one (super cool BTW if you are a fzf fan) (depends on zsh):

map / $lf -remote "send $id select $(printf '%q' $(all_files=(./*(ND)); printf '%s\n' ${all_files[@]} | fzf))"

And more...

However, as for the idea:

To be honest, lately I was thinking of getting rid of server involvement to record the names of copy/cut files to keep them in a file at all times (maybe something like ~/.local/share/lf/files) [...] Also, it could be possible to add a keybinding to review the file list in an editor. What do you think about this?

Sounds good, however I'm a bit afraid that stuff will brake if exotic file names will be marked for copy/move. What do you think @gokcehan ?

gokcehan commented 4 years ago

@ALX99 Just to be clear, I'm not talking about removing the server altogether, just the part we record file names for cut/copy (i.e. save and load commands). Server architecture is how we implement remote commands which is one of the core features of lf as we are aiming for multiple client use cases. I'm not sure what you meant about unselect. Even if you only use one client and there is no server, selecting files are a different operation than cut/copy.

@doronbehar We already assume filenames do not contain newlines. Other than that, we're simply reading and writing text in the socket, so I don't think it would change anything. What exotic filenames do you have in mind?

doronbehar commented 4 years ago

@doronbehar We already assume filenames do not contain newlines. Other than that, we're simply reading and writing text in the socket, so I don't think it would change anything. What exotic filenames do you have in mind?

Yea I know that, but still there are exotic file names without new lines that I afraid will cause trouble. Consider the file in this zip file: exotic-fnames.zip. It's reported by ls as:

-rw-rw-rw- 1 doron users 12 Jun 29 10:28 ''$'\204\230\226\200\204'' 9 - '$'\216\203\230''.txt'

OTH, when I consider the fact Golang has awesome readers and writers capabilities, I imagine it's thoroughly tested by the Golang team and I'd be surprised if we, would find faults in it...

Anyway, I don't know what to expect really, I only try to kindly suggest for such a change to be tested.

gokcehan commented 3 years ago

@doronbehar @ALX99 Sorry for taking forever to implement this. I have now made the following changes:

Server load and save commands are now removed. Instead a file is used to record file selections (e.g. ~/.local/share/lf/files).
Clients are disconnected from server on quit
Server quit command is renamed to quit! to act as a force quit by closing client connections first
A new quit command is added to only quit when there are no connected clients left
A new autoquit option is added to automatically quit server when there are no connected clients left. This option is off by default to keep the old behavior. I have added this as an option to avoid respawning server repeatedly when there is often a single client involved but more clients are spawned from time to time.
A new -single flag is added to avoid spawning and/or connecting to server on startup. Remote commands would not work in this case as the client does not connect to a server. I tried my best to add local versions of internal load and sync commands throughout the codebase. Please report back if there is anything left missing.

These changes should still be considered experimental. I think they cover every use case we discussed in this issue. Feel free to report feedback. I'm planning to make a new release when things are stable.

gokcehan commented 3 years ago

Now available in r23.

gokcehan / lf

lf -server doesn't quit automatically #177