[Windows] Shutdown function won't work

[Willie30F]

Relevant components

• Standalone tray application (based on Qt Widgets)
• Plasmoid/applet for Plasma desktop
• Dolphin integration
• Command line tool (syncthingctl)
• Integrated Syncthing instance (libsyncthing)
• Backend libraries

Environment and versions

• Version of syncthingtray: 1.1.7
• Qt version: 5.15.2
• C++ compiler (name and version): ??
• C++ standard library (name and version): ??
• Operating system (name and version): Windows 10

Bug description
When using Syncthing Tray to start syncthing automatically, I am not able to stop it with the given buttons. I can shutdown syncthing normally using the webinterface.

Steps to reproduce

1. Have syncthing running via the syncthing starter
2. Try to stop it through either ones of the stop buttons

Expected behavior
To have syncthing shutdown.

Screenshots
https://user-images.githubusercontent.com/21063653/121031512-b16cd700-c7aa-11eb-854b-870ee0bdd1f5.mp4
https://user-images.githubusercontent.com/21063653/121031530-b598f480-c7aa-11eb-9033-4bcf32a91246.mp4

Additional context
Same happens when trying to stop syncthingtray entirely:
https://user-images.githubusercontent.com/21063653/121032070-3821b400-c7ab-11eb-8d4b-817f1a7cbed5.mp4

All of this works normally on my linux laptop ;) Thanks for this great tool.

[Martchus]

I can reproduce the problem but I'm sure it wasn't always the case. There were no changes how the process is terminated on my side. Maybe Syncthing changed something about the signal handling or forking behavior under Windows. Or something within Qt changed (the bug is reproducible with Qt 5 and Qt 6).

Unfortunately Syncthing Tray's launcher doesn't use a process group (or whatever the Windows equivalent is) so it sends only SIGTERM or SIGKILL (or whatever the Windows equivalents are) to the process ID it has directly launched. This can lead to leftover processes so I'll have to check whether using a process group is possible with QProcess in a cross-platform manner.

However, it is still strange that it doesn't work even without a process group. At the point Syncthing Tray tries to stop Syncthing, the directly launched process is actually still running but unaffected by the attempt to stop it. From the error message it is clear that Syncthing Tray is trying to stop the process with the right PID. When clicking the button to kill it, it will actually be gone (leaving the other Syncthing process as leftover). That Syncthing is unaffected by the attempt to stop it could also be a bug in Syncthing.

By the way, I haven't noticed the problem so far because I usually use the built-in Syncthing under Windows and it isn't affected by the problem. So using the built-in Syncthing would also be a workaround.

[Martchus]

With process explorer it looks like there are actually 3 processes:

• syncthing.exe (low memory usage)
  • conhost.exe
  • syncthing.exe (high memory usage)

[Willie30F]

Thanks for your fast answer. Seems like a problem with many possible sources. Yeah, using the built-in syncthing would be a possibility for now, I just wanted to have auto-update functionality, that's why I set it up like that, this doesn't matter for now, because the version is the same currently. I'll just use the built-in one for now. Let's hope it's just something easy to fix 😄
If you need anything additionally, let me know, I'll be happy to test it out.
However, thanks again for your fast answer and keep up the good work 🥇

[Martchus]

It looks like the most reasonable approach is to use Boost.Process. I've been using it already under GNU/Linux and it makes it very easy to create process groups. I've looked at its code and it also does some magic under Windows when one creates a process group there so it'll likely solve this issue (without adding platform-specific code to Syncthing Tray itself). However, it'll be a lot of refactoring required to exchange QProcess with Boost.Process so it'll take a while to implement it.

[tomasz1986]

With process explorer it looks like there are actually 3 processes:

• syncthing.exe (low memory usage)

  • conhost.exe
  • syncthing.exe (high memory usage)

Probably related: syncthing/syncthing#7042 (see the first answer).

[Willie30F]

[...] However, it'll be a lot of refactoring required to exchange QProcess with Boost.Process so it'll take a while to implement it.

That's fine, using the in-built syncthing works atm.

With process explorer it looks like there are actually 3 processes:

• syncthing.exe (low memory usage)

  • conhost.exe
  • syncthing.exe (high memory usage)

Probably related: syncthing/syncthing#7042 (see the first answer).

I actually tried removing that option before, but it didn't solve the problem for me either.

[Martchus]

Looks like using Boost.Process works just fine. It easily allows using a job object under Windows (see https://www.boost.org/doc/libs/1_76_0/doc/html/process/reference.html#header.boost.process.group_hpp for details) which solves the problem of terminating any sub processes spawned by the initially started process.

(Due to #95 I cannot provide a stable build to test currently.)

[Martchus]

Oh, and apparently there's no equivalent for SIGTERM under Windows. QProcess which was/is used in Syncthing Tray so far sends WM_CLOSE but we saw that this isn't working here (and Qt's documentation also mentions the caveat: https://doc.qt.io/qt-5/qprocess.html#terminate).

So I just terminate if forcefully under Windows for now. Maybe it would be better to try terminating it via the REST-API first and only stop it on "process level" if that doesn't work.

[Willie30F]

So I just terminate if forcefully under Windows for now. Maybe it would be better to try terminating it via the REST-API first and only stop it on "process level" if that doesn't work.

This sounds like the best solution to this :)

[Martchus]

I've now made it use the REST-API under Windows, at least when one clicks the "Stop" button on the tray window for a local connection. (The "Stop" button within the settings is a bit more difficult because from that context it isn't always clear which connection is used because one can spawn multiple tray icons connecting to different instances.)

Note that this change should also be an improvemnt under Linux because now all sub-processes are terminated/killed cleanly with no leftovers, regardless how many processes Syncthing might fork under the hood.

Links to the latest development build can be found here: #95 (comment)

[Willie30F]

Sorry for the late answer, currently not at home. Thanks for the fix, keep it up.

[Martchus]

The "Stop" button within the settings is a bit more difficult because from that context it isn't always clear which connection is used because one can spawn multiple tray icons connecting to different instances.

This should be fixed with 4c6315b. (Of course if none of the configured connections match the Syncthing process it cannot be stopped via the REST-API and will still not be terminated gracefully.)

[Willie30F]

Yeah, all of it works fine under windows now. Now syncthingtray won't start on my linux anymore, with an libboost error despite it's installed. But I will open another ticket for this. Thanks again.

[Martchus]

Please don't open a new issue unless you're sure it is not just #98.

[Willie30F]

Oups, thanks, already rebuilding it :) Thanks anyways 😃

[Martchus]

I think it works in the best way Windows allows. It'll now just uses the REST-API to trigger gracefull termination under Windows (as there's no SIGTERM) and killing works also for all sub processes by using a process group (or this job thing under Windows).