jellyfin / jellyfin-server-windows

The Windows tray app and installer for Jellyfin Server on Windows.
https://jellyfin.org
MIT License
80 stars 27 forks source link

With basic installation, the tray app always kills Jellyfin instead of safely stopping it #81

Open qwerty12 opened 1 year ago

qwerty12 commented 1 year ago
Version: 10.8.9
Operating System: Windows 11 21H2
Architecture: X64

Hi,

First of all, many thanks to you and the Jellyfin team for a wonderful media server.

I install Jellyfin in the recommended Basic Install mode. I noticed that whenever stopping Jellyfin via the tray app, C:\ProgramData\Jellyfin\Server\data would still contain jellyfin.db-shm and jellyfin.db-wal, indicating there was changes yet to be still written to jellyfin.db but couldn't occur because it was killed. Conversely, shutting down Jellyfin with the button on JF's dashboard appears to shut it down cleanly; the extra files mentioned previously aren't in that folder.

I see that the tray app tries to close the main window before killing the process, but Jellyfin has no window to send the message to (and even then, Jellyfin itself needs a message handler for WM_CLOSE), so it will always get killed. I changed the close mechanism to send Ctrl+C here instead and it works, but my code is crude and the reliability of the console APIs always seem to vary from Windows version to Windows version. (As an aside, another commit to the same repo adds a very-hacky-implemented listener for shutdown events so that Jellyfin has about ~10 seconds to safely exit before my PC finally gets powered off.)

It's my understanding that this won't happen in the service mode, because NSSM will also send Ctrl+C to Jellyfin. Thanks again.

anthonylavado commented 1 year ago

Oooh. Thank you for the example there. I've been meaning to add this in, and to be honest, as long as the commands work in Windows 10+, I think we're okay.

We're moving to .NET 7 with the next bigger release (10.9), so the server support will be for these OSes: https://github.com/dotnet/core/blob/main/release-notes/7.0/supported-os.md

Granted, this app is made with .NET Framework, but that's to avoid having to build & ship a bunch of data for what is essentially a tiny utility.

qwerty12 commented 1 year ago

Oooh. Thank you for the example there. I've been meaning to add this in, and to be honest, as long as the commands work in Windows 10+, I think we're okay.

I think you should indeed be okay - not that I really know what I'm talking about here. I had problems some years ago with doing the same thing for another program but on reflection, I think that was a me problem, rather than one caused by Windows.

For what it's worth, I've been using the modified tray app that shuts down Jellyfin with GenerateConsoleCtrlEvent for a month and a bit now and I've not run into any problems. The only problem I can think of is that sending Ctrl+C isn't technically guaranteed to shut down Jellyfin, so I ended up adding bad code in the tray app to kill Jellyfin after ten seconds but I don't think I've noticed it even being hit in practice.

We're moving to .NET 7 with the next bigger release (10.9), so the server support will be for these OSes: https://github.com/dotnet/core/blob/main/release-notes/7.0/supported-os.md

Save for communicating with Jellyfin via the REST API, GenerateConsoleCtrlEvent might indeed be the best way to cleanly shutdown Jellyfin IMO, as Jellyfin itself already handles Ctrl+C cleanly so no changes are required there, plus GenerateConsoleCtrlEvent etc. is provided by Windows itself, going as far back as NT 4 I think, and I can't imagine .NET's DllImport functionality ever going away.

Ede123 commented 5 months ago

As a thought: Does it even make sense to stop Jellyfin when closing the tray app?

For me I'd see the tray app only as a graphical frontent for the server running in the background. When I close the graphical frontend, there's no need to stop the server at all.

As a matter of fact I just came here when I accidentally closed the tray app and was wondering why Jellifin had shut down as well.

anthonylavado commented 5 months ago

@Ede123 Yes, it does make sense. There is no built in "shutdown" button for the web interface on Windows (unlike Linux) because it requires being able to run a script and a few other wrinkles we couldn't really sort out in Windows.

We can investigate it, but I don't feel sure that it will make it into this next release for the end of April (there's quite a lot to cover across all projects).

If you wanted a background service not tied to the tray app today, you'd need Jellyfin to be running in the service mode. It's a choice made during the installation, and there is no good way to convert from one to the other. It's labelled as "Advanced" because involves a lot of unique permission challenges for accessing network modes.

Could I suggest hiding the tray app from the notification area in the meanwhile? 🙂

Ede123 commented 5 months ago

Thanks, that does make sense.

Personally I'm fine with the current solution, just wanted to note that it might be counter-intuitive when compared to some other server applications (also on windows), and that it might cause "accidents" Like in my case, when I wanted to close another app from the tray and accidentally killed the Jellyfin server instead.

It's a good pointer that the service would behave differently - maybe I'll actually try that at one point. (Didn't dare to do so, since it's specifically recommended against in multiple places, but it might actually align more with what I'd personally be expecting personally in a server app.

As a thought: Maybe The tray app could control Jellyfin via some sort of RPC? Then it could maybe be decoupled from the server itself and shutting down the server via other means might be easier as well?

In any case, certainly none of these is particularly urgent (functionality is all there as it is)! Thanks for the great software!

anthonylavado commented 4 months ago

@qwerty12 Looking at your fork, you seem to have a lot of nice changes, like holding at shutdown/etc. Would you be open to making a pull request? Keep in mind version 10.9 is now being released so the files are... different. Some updates would be needed.

qwerty12 commented 4 months ago

@anthonylavado

Oh, congratulations on the release :-) I think I'll end up rebasing anyway, I messed up the merging.

Fair warning: the code quality of my changes isn't exactly top notch

Leaving out changes that make sense only for me, and those that rely on heavily on .NET internals remaining the same for not a lot of benefit, I think these might be worth looking at:

Small changes:

Larger changes:

Opinionated changes:

Let me know what to discard, and I'll try and make PRs for the rest.

EDIT: Changes rebased on master: https://github.com/qwerty12/jellyfin-server-windows/tree/10.9-rebase