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

Properly handle SIGHUP and SIGTERM #305

Merged
merged 5 commits into from
Apr 13, 2020

Conversation

Provessor
Copy link
Contributor

@Provessor Provessor commented Mar 11, 2020

Properly handle exit signals SIGHUP and SIGTERM to avoid exiting prematurely.

Fixes #300, #292

To preserve system independence, according to the documentation, when Windows sends CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT or CTRL_SHUTDOWN_EVENT, Notify will return SIGTERM like a *nix system.

@gokcehan
Copy link
Owner

@Provessor Thanks for working on this.

Messages sent to the quit channel are sometimes ignored (i.e. when a copy/move operation is in progress). Have you tested this patch to see what happens in this case?

Also do you need all three signals or SIGHUP/SIGTERM is enough for #292 and #300 ? Interrupt signal should ideally be handled as part of #162 . It would be great if you're willing to work on this but if not maybe we should simply exclude that signal from this patch.

@Provessor
Copy link
Contributor Author

Provessor commented Mar 20, 2020

@gokcehan If SIGINT is going to be handled more specifically elsewhere I'll remove it from this.
Edit: removed

As of the last commit, quitChan will not halt the exit if a copy/move/delete operation is in progress on SIGHUP/SIGTERM. I have read through the copy/move/delete code and couldn't see anything from there that needed explicit handling.

@gokcehan
Copy link
Owner

@Provessor You removed syscall.SIGINT but os.Interrupt is still there. How does that work?

I'm not sure if it's a good idea to cancel the copy/move operation altogether. It may be better to keep the operation going in the background. Do you know what is happening to copy/move operations when you close the terminal with the current code?

@Provessor
Copy link
Contributor Author

@gokcehan my mistake leaving os.Interrupt, I was very tired.

In go the main goroutine must be running for any other goroutines to run. This means when loop() returns after receiving true from quitChan all other goroutines will be terminated and the current operation won't be continued after that point because the corresponding goroutine doesn't exist anymore.

My thought process was if the operation continued after the terminal is disconnected then that could lead to unexpected results, a directory which is being duplicated without an obvious source. Personally I don't think continuing the operation in the background should be the default behaviour, however it's up to you.
The best way to continue the operation in that case might be to move it into a 'server' process or somehow share the current copy/move/delete status to all instances so a new/other instance can still report on it.

@gokcehan
Copy link
Owner

gokcehan commented Apr 1, 2020

@Provessor So I have tried the current code and it seems that copying is finished when the terminal is closed.

For now, I think I agree it is better to finish the copying operation and leave background copying to a future possible server implementation as you suggest.

The way goroutines are handled in the patch still feels strange though. Do we want to maintain a separate list of cleanup operations in here? If you agree as well, maybe we should just try to quit, by sending a quit signal as in your first patch, and leave copy case handling as before, by leaving a log file behind. I don't think we will have complaints in the related issues as it will handle the common use case properly and the other case is mostly unintentional.

@Provessor
Copy link
Contributor Author

@gokcehan I have done some more testing and it seems that there is a gap between window close and copy stopping (at least on my machine) however this is only noticeable with large files (~1G+).
I agree with leaving a log file if a copy was terminated however is letting the system terminate lf with an operation in progress the best way to do this?

I've since changed my mind on this since my last comment; IMHO the best solution to this would be to perform all copy/move/delete operations on a server process (which could then also take over preview caching, etc.) and I would be happy to leave a zombie process to finish these operations with this PR with intentions to pick up on it later.

@Provessor Provessor changed the title Properly handle SIGHUP, SIGINT and SIGTERM Properly handle SIGHUP and SIGTERM Apr 3, 2020
@Provessor
Copy link
Contributor Author

@gokcehan it's not possible to daemonize a process in Go (see this or this), I've found one cross-platform package godaemon that uses exec() to achieve this task however it seems to be no longer maintained and it is "forced to run the process all over again, from the start". Without properly daemonizing the operations will always be halted when the parent terminal exits.

It is possible to start a new process in the background in pure Go (something like this) so maybe this should be left for a server process implementation?

@gokcehan
Copy link
Owner

@Provessor Yes I think it is better to leave it for a later patch. I'm merging this as it is now. Thanks for the patch.

@gokcehan gokcehan merged commit c49140a into gokcehan:master Apr 13, 2020
@Provessor Provessor deleted the signal-handler branch April 14, 2020 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log files remain in /tmp if closed by window manager.
2 participants