search

Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.68k stars 44 forks source link

[Windows] Shutdown function won't work #94

Closed Willie30F closed 3 years ago

Willie30F commented 3 years ago

Relevant components

Environment and versions

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 commented 3 years ago

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 commented 3 years ago

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

Willie30F commented 3 years ago

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 :smile: 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 :1st_place_medal:

Martchus commented 3 years ago

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 commented 3 years ago

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: https://github.com/syncthing/syncthing/issues/7042 (see the first answer).

Willie30F commented 3 years ago

[...] 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 commented 3 years ago

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 https://github.com/Martchus/syncthingtray/issues/95 I cannot provide a stable build to test currently.)

Martchus commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/Martchus/syncthingtray/issues/95#issuecomment-869215802

Willie30F commented 3 years ago

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

Martchus commented 3 years ago

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 https://github.com/Martchus/syncthingtray/commit/4c6315b4509b5247f69d3f60a7980932a1b942fc. (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 commented 3 years ago

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 commented 3 years ago

Please don't open a new issue unless you're sure it is not just https://github.com/Martchus/syncthingtray/issues/98.

Willie30F commented 3 years ago

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

Martchus commented 3 years ago

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).