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

Fix Duplicati shutdown issue on Synology #3567

Merged
merged 2 commits into from Dec 28, 2018

Conversation

Projects
None yet
5 participants
@drwtsn32x
Copy link
Contributor

drwtsn32x commented Dec 27, 2018

The current Synology start-stop-status does not properly shut down Duplicati. It terminates only the main process but not the child processes.

Example processes before Duplicati is stopped via Synology Package Manager:

bash-4.3# ps ax | grep Dup
15236 ?        Sl     0:00 mono /volume1/@appstore/Duplicati/Duplicati.Server.exe
15242 ?        Sl     0:01 /volume1/@appstore/Mono/usr/local/bin/mono-sgen /volume1/@appstore/Duplicati/Duplicati.Server.exe

After telling Duplicati to stop, only PID 15236 is terminated. PID 15242 remains, and this causes problems if you try to start Duplicati again. This issue also causes problems when people try to upgrade as a stop/start cycle is needed in such a case.

With this change the script will look up the process group ID (PGID) of the main PID and issue a kill command to that whole group. This will ensure the child processes are also shut down.

@duplicatibot

This comment has been minimized.

Copy link

duplicatibot commented Dec 27, 2018

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/2-0-4-5-server-appears-to-be-broken-on-synology/5576/23

@verhoek

This comment has been minimized.

Copy link
Contributor

verhoek commented Dec 27, 2018

Hi, thanks for your PR. Can you change it to use pkill -P $PID ?

@drwtsn32x

This comment has been minimized.

Copy link
Contributor

drwtsn32x commented Dec 27, 2018

Doesn't "pkill -P" only match the child processes for a given PID? What about the main process itself? Or maybe we should use "pkill -g $PGID" instead as that also works on PGIDs like "kill -- -$PGID"

@drwtsn32x

This comment has been minimized.

Copy link
Contributor

drwtsn32x commented Dec 27, 2018

Disregard - I just tested pkill -P $PID and it did properly kill the PID and its child processes. I agree it is cleaner. I'll update the pull request. Thanks!

@drwtsn32x

This comment has been minimized.

Copy link
Contributor

drwtsn32x commented Dec 27, 2018

I've never updated a pull request before. Hopefully I did it correctly.

@verhoek

This comment has been minimized.

Copy link
Contributor

verhoek commented Dec 28, 2018

That is better, thanks for changing it. pkill apparently works only for direct children but that is the case here. I'm wondering why mono-sgen (a garbage collector) keeps running when the parent process is killed - I hoped there would be a more natural way of stopping them both, like mono-sgen would notice the main mono process is gone. I noticed such things on Ubuntu while debugging code, but that's a bit different.

I think it's safe to kill the mono-sgen, any memory the processes occupied will be freed by the kernel.
I wonder if hanging mono-sgen's also happens on other platforms.

@kenkendk, @warwickmm, @Pectojin : does one of you know more about this?

@Pectojin
Copy link
Member

Pectojin left a comment

pkill -P seems to work fine when I test it on my RHEL laptop, killing the main process and reaping the children. Assuming it works identically on DSM, I think this is the right solution.

Killing mono-sgen is not an issue as pklill will send SIGTERM so mono-sgen will shut down properly. Even if it doesn't the kernel will reclaim anything left as you pointed out.

@Pectojin Pectojin merged commit 9dae926 into duplicati:master Dec 28, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ts678

This comment has been minimized.

Copy link

ts678 commented Jan 9, 2019

Does anyone else feel like this is actually a workaround for Duplicati parent not catching the kill and shutting down its child? That's what a start-stop script normally expects, as far as I know. Issue was raised in forum in Running Duplicati on startup on Freenas / FreeBSD, which wound up with Synology's change in a BSD script.

There's a similar issue on Windows, but not Linux, with Control-C on Server leaving the child Server running. Linux is probably killing the tty foreground process group, and actually I favored that approach for Synology (more thorough, more future-proof, but probably less deterministic in shutdown sequence), but -P is decent, EXCEPT that it needs to be put in everything (including third-party) to make up for non-standard behavior...

@duplicatibot

This comment has been minimized.

Copy link

duplicatibot commented Jan 9, 2019

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/2-0-4-5-server-appears-to-be-broken-on-synology/5576/47

@drwtsn32x

This comment has been minimized.

Copy link
Contributor

drwtsn32x commented Jan 10, 2019

Does anyone else feel like this is actually a workaround

Yes, I do. I'm not sure what is responsible for the root problem of not terminating the child process. Is it Mono's responsibility? Duplicati's?

If someone can identify the root cause and fix it, then this workaround should be un-done. In the meantime the workaround is better than nothing, and there doesn't seem to be any negative consequences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment