Chiv2-Community / UnchainedLauncher

GNU General Public License v3.0
8 stars 1 forks source link

Series/1.x.x #50

Open Jacoby6000 opened 1 year ago

Jacoby6000 commented 1 year ago

Do not merge. Just here as a quick reference to compare with 0.x.x

ARoese commented 1 month ago

Are there any other features we are going to want to add to this, or the next step going to be merging this to main?

Jacoby6000 commented 1 month ago

We still need to port over a couple of fixes from master in to this, like the sig files thing. We also need to validate that everything works as expected. Iirc there are still a couple of bugs. After that we can probably merge it.

After we merge it I need to write some tests for the mod management (ideally I would've done this already and maybe I will as a part of validation)

And then ideally this will all be ready and we can build a modern UI further down the road

ARoese commented 1 day ago

https://github.com/Chiv2-Community/UnchainedLauncher/blob/f6e576fed2b213af1d5bd71d864e439f0eeb656d/UnchainedLauncherGUI/src/ViewModels/ServersViewModel.cs#L28

this can't currently be shared. The current Backend implementation stores the key of the last server it registered, so all registered servers are using the same key if this is shared. This is a failure of the implementation on my part (too hasty making it OO) and should be resolved when I clean things up. The interface will change, though, because then key needs to be passed back to any function requiring one. This gets much easier to manage the way I have it set up locally (will push and merge later)

As for implementation, it's extremely annoying that some of the httpClient HTTP methods do not allow you to set headers as part of the call. I'll need to go back to my hackey workaround to do this. Setting the default header temporarily to the key is bad because that breaks thread-safety. (keys could get mixed up between concurrent calls)

ARoese commented 1 day ago

https://github.com/Chiv2-Community/UnchainedLauncher/blob/f6e576fed2b213af1d5bd71d864e439f0eeb656d/UnchainedLauncherGUI/src/ViewModels/ServersViewModel.cs#L80-L81

Backends are not IDisposable and hold no unmanaged or managed resources that need to be cleaned up deterministically, so they don't need to be disposed or IDisposable.

Backends aren't IDisposable here, but that will change when I properly implement them

ARoese commented 4 hours ago

https://github.com/Chiv2-Community/UnchainedLauncher/blob/f6e576fed2b213af1d5bd71d864e439f0eeb656d/UnchainedLauncherGUI/src/ViewModels/ServersViewModel.cs#L30-L39

This is dangerous because it disposes a backend before the servers using it are disposed. This will cause those servers to call functions on the disposed backend and die prematurely with no way to recover in an unclean way. I think the servers using a backend should have shared ownership of it. When the last server using a backend disposes, it should dispose the backend as well.

We can do this one of two ways:

  1. Do not dispose the backend(s) so the garbage collector figures out when this has happened and disposes it
    • super easy
    • nondeterministic, but we don't make that many backends so having that resource not get cleaned up in a timely manner is not that bad
      1. Use something like this https://github.com/dotnet/runtime/issues/24990
        • have to do stuff
        • the correct thing to do, tbh

This is also not thread-safe when I think it technically should be, (it's getting used by UI event threads) but the race condition occurs only when there is an inhumanly fast UI interaction.